From 482884b2363e911ea49138a043748be103bde523 Mon Sep 17 00:00:00 2001 From: KK <37928252+dragonknightk@users.noreply.github.com> Date: Fri, 30 Oct 2026 21:49:09 +0800 Subject: [PATCH] Keep the parent snapshot id when removing snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes issue 4193 # Rationale for this change Applying `RemoveSnapshotsUpdate` set `parent_snapshot_id` to `None` on every snapshot whose parent was removed. Since #3512, an incremental append scan accepts an expired exclusive start if some ancestor of the end snapshot still has it as its parent, so this reset makes that case fail after `expire_snapshots`. For example: - append three times, then expire the first snapshot with `expire_snapshots().by_id(...)` - `incremental_append_scan(from_snapshot_id_exclusive=)` raises `Starting snapshot (exclusive) ... is not a parent ancestor of end snapshot ...` instead of returning the rows of the second and third appends The reset came in with #1561; it was not discussed in review there, and the Java code linked in that review keeps the parent. Java keeps the parent id: `TableMetadata.Builder.rewriteSnapshotsInternal` only drops the removed snapshots from the list and leaves the remaining ones unchanged. The spec does not ask for the parent to still exist either; the field description of `parent-snapshot-id` only says "Omitted for any snapshot with no parent". ### What changed - the `RemoveSnapshotsUpdate` handler in `pyiceberg/table/update/__init__.py` drops the removed snapshots and keeps the remaining ones as they are - a dangling parent id is already handled by `ancestors_of`, which stops when `snapshot_by_id` returns `None`, the same way as Java's `SnapshotUtil.ancestorsOf` The scan from the example now returns the rows of the second and third appends (｀・ω・´) ## Are these changes tested? Yes, in `tests/table/test_init.py`: - `test_incremental_append_scan_from_snapshot_expired_by_maintenance` runs the example against the memory and SQL catalogs and checks that the scan returns `[20, 30]`. Without this change, the scan raises the `ValueError` above - `test_update_remove_snapshots` asserted the old behavior (`parent_snapshot_id is None`). It now asserts that the remaining snapshot keeps its parent id from the fixture, and fails without this change with `assert None == 3051729675574597004` ## Are there any user-facing changes? Yes, a bug fix: after `RemoveSnapshotsUpdate` is applied in pyiceberg, for example by `expire_snapshots`, the remaining snapshots keep their `parent_snapshot_id`, even if that snapshot no longer exists in the metadata. This applies to catalogs that apply the update in pyiceberg, such as SQL, Glue and Hive; for REST catalogs, the server applies it. Parent ids that were already cleared before this fix are not restored. Code that read `parent_snapshot_id is None` to find the oldest remaining snapshot should check whether `snapshot_by_id` returns `None` instead (・ω・)b ## AI Disclosure Human-led contribution. Grok Bot, working for dragonknightk, assisted with this change. All submitted content was reviewed by a human before submission. --- pyiceberg/table/update/__init__.py | 10 +--------- tests/table/test_init.py | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pyiceberg/table/update/__init__.py b/pyiceberg/table/update/__init__.py index aa1a09ebb2..58823501d4 100644 --- a/pyiceberg/table/update/__init__.py +++ b/pyiceberg/table/update/__init__.py @@ -535,15 +535,7 @@ def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _Tab if not any(snapshot.snapshot_id == remove_snapshot_id for snapshot in base_metadata.snapshots): raise ValueError(f"Snapshot with snapshot id {remove_snapshot_id} does not exist: {base_metadata.snapshots}") - snapshots = [ - ( - snapshot.model_copy(update={"parent_snapshot_id": None}) - if snapshot.parent_snapshot_id in update.snapshot_ids - else snapshot - ) - for snapshot in base_metadata.snapshots - if snapshot.snapshot_id not in update.snapshot_ids - ] + snapshots = [snapshot 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 diff --git a/tests/table/test_init.py b/tests/table/test_init.py index f6b984f1a3..e4f72c41bb 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -559,6 +559,26 @@ def test_incremental_append_scan_admits_expired_from_snapshot(example_table_meta assert scan._validate_and_resolve_snapshots() == (expired_snapshot_id, remaining_snapshot_id) +def test_incremental_append_scan_from_snapshot_expired_by_maintenance(catalog: Catalog) -> None: + # Same as the test above, but the snapshot is removed by `expire_snapshots`, which must keep + # the parent id of the remaining child so that the expired `from` is still accepted. + import pyarrow as pa + + catalog.create_namespace("default") + table = catalog.create_table("default.expired_from_snapshot", schema=pa.schema([("n", pa.int64())])) + appended_snapshot_ids: list[int] = [] + for n in (10, 20, 30): + table.append(pa.table({"n": [n]})) + appended_snapshot_ids.append(table.current_snapshot().snapshot_id) # type: ignore[union-attr] + + table.maintenance.expire_snapshots().by_id(appended_snapshot_ids[0]).commit() + + assert table.snapshot_by_id(appended_snapshot_ids[0]) is None + # Only the rows appended after the expired `from` snapshot, i.e. by the second and third appends. + result = table.incremental_append_scan(from_snapshot_id_exclusive=appended_snapshot_ids[0]).to_arrow() + assert sorted(result["n"].to_pylist()) == [20, 30] + + def test_incremental_append_scan_update_preserves_type(table_v2: Table) -> None: # `update()` lives on BaseScan and uses `type(self)(...)` to construct the copy; # verify the concrete type is preserved through both `update` and the fluent helpers. @@ -1169,7 +1189,8 @@ def test_update_remove_snapshots(table_v2: Table) -> None: new_metadata = update_table_metadata(table_v2.metadata, (update,)) assert len(new_metadata.snapshots) == 1 assert new_metadata.snapshots[0].snapshot_id == KEEP_SNAPSHOT - assert new_metadata.snapshots[0].parent_snapshot_id is None + # REMOVE_SNAPSHOT is KEEP_SNAPSHOT's parent in the fixture; the parent id is kept even though it now dangles + assert new_metadata.snapshots[0].parent_snapshot_id == REMOVE_SNAPSHOT assert new_metadata.current_snapshot_id == KEEP_SNAPSHOT assert new_metadata.last_updated_ms > table_v2.metadata.last_updated_ms assert len(new_metadata.snapshot_log) == 1