Preserve a ref's retention policy when its snapshot moves - #3770
Open
1fanwang wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a retention-policy regression in the snapshot commit/write path: when writing a new snapshot to an existing branch, the branch’s retention configuration (max_ref_age_ms, max_snapshot_age_ms, min_snapshots_to_keep) is now preserved instead of being dropped by the emitted set-snapshot-ref update.
Changes:
- Preserve existing branch retention fields when
_SnapshotProducer._commit()updates a branch ref during writes. - Add unit tests validating retention is preserved across multiple writes and remains unset when not configured.
- Add an integration test covering the REST catalog scenario where branch retention previously regressed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pyiceberg/table/update/snapshot.py |
Carries existing ref retention fields into the SetSnapshotRefUpdate emitted during snapshot commits to a branch. |
tests/table/test_manage_snapshots.py |
Adds unit coverage to ensure branch retention survives repeated writes and remains unset if not configured. |
tests/integration/test_writes/test_writes.py |
Adds an integration regression test for preserving branch retention on repeated writes via REST catalog. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
create_branch() accepts max_ref_age_ms, max_snapshot_age_ms and min_snapshots_to_keep, but any later operation that moves the ref drops all three. Both paths that move a ref rebuild it from a SetSnapshotRefUpdate that omits the retention fields, so the applied ref falls back to the defaults: - writing to a branch, via _SnapshotProducer._commit() - set_current_snapshot(), and the rollbacks that delegate to it A branch created with a retention policy therefore loses it on its first write, which is the point at which the policy starts to matter. Carry the existing ref's retention fields through on both paths, mirroring Java's TableMetadata.Builder.setBranchSnapshotInternal, which rebuilds the moved ref with SnapshotRef.builderFrom(ref, replacementSnapshotId). create_tag() and create_branch() still set the fields explicitly, so a ref with no policy does not acquire one. Signed-off-by: 1fanwang <1fannnw@gmail.com>
1fanwang
force-pushed
the
preserve-branch-retention
branch
from
August 8, 2026 06:41
198ed00 to
4f48ea4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
create_branch()acceptsmax_ref_age_ms,max_snapshot_age_msandmin_snapshots_to_keep, and any later operation that moves the ref drops all three.The branch loses its retention policy at the moment the policy starts to matter — a branch is created to hold a staged write, and the staged write is what erases it.
Two paths move a ref, and both rebuild it from a
SetSnapshotRefUpdatethat omits the retention fields, so_apply_table_updatefalls back to the defaults:_SnapshotProducer._commit()set_current_snapshot(), and the rollbacks that delegate to itSetSnapshotRefUpdatealready carries all three fields; nothing was populating them.Java preserves them.
TableMetadata.Builder.setBranchSnapshotInternalrebuilds the moved ref withSnapshotRef.builderFrom(ref, replacementSnapshotId), which copies the existing retention config forward. BothsetBranchSnapshot(writes) androllbackToroute through it.This reads the existing ref and carries its fields through on both paths.
create_tag()andcreate_branch()still set the fields explicitly from their arguments, so a ref with no policy does not acquire one.#3649 fixes the same class of bug on the fast-forward path. It does not touch either path here, so the two are complementary.
Are these changes tested?
Integration, against the REST catalog from
dev/docker-compose-integration.yml: create a branch with all three retention fields, write to it twice, assert the policy survives and thatmaingains nothing.Red, with
pyiceberg/table/update/snapshot.pyat upstream/mainThe unit tests fail the same way on both paths:
Green, with the change restored
The 6 that pass red as well as green are the "a ref without a policy stays without one" cases, which pin that this does not invent defaults.
Whole unit and integration suites pass;
prek run -aclean.Are there any user-facing changes?
A ref keeps the retention policy it was created with when its snapshot moves. No API change.