Skip to content

Keep the parent snapshot id when removing snapshots - #4206

Open
dragonknightk wants to merge 1 commit into
apache:mainfrom
dragonknightk:fix/4193-keep-parent-on-remove-snapshots
Open

dragonknightk wants to merge 1 commit into
apache:mainfrom
dragonknightk:fix/4193-keep-parent-on-remove-snapshots

Conversation

@dragonknightk

Copy link
Copy Markdown

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.

Closes issue 4193

# Rationale for this change

Applying `RemoveSnapshotsUpdate` set `parent_snapshot_id` to `None` on every snapshot whose parent was removed. Since apache#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 apache#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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

incremental_append_scan from an expired start fails after expire_snapshots

1 participant