rambleraptor commented on code in PR #4212:
URL: https://github.com/apache/iceberg-python/pull/4212#discussion_r4429374666
##########
pyiceberg/table/update/__init__.py:
##########
@@ -544,11 +544,14 @@ def _(update: RemoveSnapshotsUpdate, base_metadata:
TableMetadata, context: _Tab
for snapshot in base_metadata.snapshots
if snapshot.snapshot_id not in update.snapshot_ids
]
- snapshot_log = [
- snapshot_log_entry
- for snapshot_log_entry in base_metadata.snapshot_log
- if snapshot_log_entry.snapshot_id not in update.snapshot_ids
- ]
+ expired_ids = set(update.snapshot_ids)
+ snapshot_log: list[SnapshotLogEntry] = []
+ for entry in base_metadata.snapshot_log:
+ if entry.snapshot_id in expired_ids:
Review Comment:
```suggestion
if entry.snapshot_id not in {s.snapshot_id for s in snapshots} :
```
As it stands, this only works on expired ids. Java also works on situations
when the snapshot no longer exists in the table.
##########
tests/table/test_expire_snapshots.py:
##########
@@ -14,18 +14,69 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import json
import threading
-from datetime import datetime, timedelta
+from datetime import UTC, datetime, timedelta
+from types import SimpleNamespace
from unittest.mock import MagicMock, Mock
from uuid import uuid4
import pytest
+from pyiceberg.catalog import Catalog
from pyiceberg.table import CommitTableResponse, Table
from pyiceberg.table.update import RemoveSnapshotsUpdate, update_table_metadata
from pyiceberg.table.update.snapshot import ExpireSnapshots
[email protected]("expiration", ["by_id", "older_than"])
+def test_expiration_does_not_substitute_older_history(catalog: Catalog,
monkeypatch: pytest.MonkeyPatch, expiration: str) -> None:
+ import pyarrow as pa
+
+ catalog.create_namespace("history")
+ table = catalog.create_table("history.events", schema=pa.schema([("x",
pa.int64())]))
+ # Keep snapshot and metadata timestamps consistent; commits and file IO
remain real.
+ clock_ms = table.metadata.last_updated_ms
+ monkeypatch.setattr("pyiceberg.table.snapshots.time",
SimpleNamespace(time=lambda: clock_ms / 1000))
Review Comment:
Do we need to monkeypatch for these? I'd really like to avoid monkeypatching
whenever possible.
##########
pyiceberg/table/update/__init__.py:
##########
@@ -544,11 +544,14 @@ def _(update: RemoveSnapshotsUpdate, base_metadata:
TableMetadata, context: _Tab
for snapshot in base_metadata.snapshots
if snapshot.snapshot_id not in update.snapshot_ids
]
- snapshot_log = [
- snapshot_log_entry
- for snapshot_log_entry in base_metadata.snapshot_log
- if snapshot_log_entry.snapshot_id not in update.snapshot_ids
- ]
+ expired_ids = set(update.snapshot_ids)
+ snapshot_log: list[SnapshotLogEntry] = []
+ for entry in base_metadata.snapshot_log:
+ if entry.snapshot_id in expired_ids:
Review Comment:
Actually, might be better to set this to a variable to avoid constantly
recalculating it.
##########
tests/table/test_init.py:
##########
@@ -1186,6 +1186,46 @@ def test_update_remove_snapshots_doesnt_exist(table_v2:
Table) -> None:
update_table_metadata(table_v2.metadata, (update,))
[email protected]("table", [lf("table_v1"), lf("table_v2"),
lf("table_v3")])
+def test_remove_snapshots_retains_contiguous_history(table: Table) -> None:
+ snapshots = [
+ Snapshot(snapshot_id=snapshot_id, timestamp_ms=snapshot_id,
manifest_list=f"file:///manifest-{snapshot_id}.avro")
+ for snapshot_id in (10, 20, 30, 40)
+ ]
+ cases: list[tuple[str, list[int], list[int], list[int]]] = [
Review Comment:
This should probably be parameterized. That being said, I think it'll make
the test pretty messy.
What would you think about having multiple tests + a helper? I'll be honest,
my eyes are glazing over reading all of these integers and trying to make sure
that these all seem valid 😆
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
