dragonknightk opened a new pull request, #4206: URL: https://github.com/apache/iceberg-python/pull/4206
Closes #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=<first snapshot id>)` 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. -- 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]
