Repository navigation
Core: Retry appends on catalogs that only expose the current snapshot - #4333
Open
Anurag-S1ngh wants to merge 1 commit into
Open
Anurag-S1ngh wants to merge 1 commit into
Anurag-S1ngh wants to merge 1 commit into
Conversation
AP24110011695
approved these changes
Nov 10, 2026
AP24110011695
left a comment
There was a problem hiding this comment.
Verified locally (Windows, Python 3.12):
- Full
tests/table/test_commit_retry.pypasses on this branch — 129 passed, including the three new history-less-catalog tests across all catalog fixtures. - Fail-first reproduced: swapping only the
pyiceberg/source to main makes exactly those new tests fail (9 failed across fixtures) with theValidationExceptionbehavior from #4327 — so the tests genuinely pin the regression. - Traced the class structure to confirm the fix lands in the right place: the
base_missingguard sits in_SnapshotProducer._validate_concurrency, and_FastAppendFilesoverrides that method with a no-op, so appends retry without validation (matching Java'sSnapshotProducerbehavior) while deletes/overwrites inherit the guarded path and raiseCommitFailedExceptionwith a message that names the missing snapshot. The only newbase=Nonecase (base_idset but snapshot missing) is intercepted beforestarting_snapshotis used; the pre-existingbase_id=Nonepath with a non-empty window is untouched. resolve()no longer raises,is_empty()docstring matches the new semantics, and the error text distinguishes a lost race from a data conflict — the pre-0.12 behavior, restored.
Good catch on a real 0.12 regression for Nessie-style catalogs, and the test design (hiding the starting snapshot from refreshed metadata) is a neat way to emulate them. Nice work.
This branch has not been deployed
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
Some REST catalogs expose only the current snapshot and keep table history elsewhere, Nessie being the main example.
CommitWindow.resolveraisedValidationException: Cannot find starting snapshot <id>whenever the starting snapshot was missing after a refresh, so since the retry added in #3320 no commit loser could ever retry on those catalogs, not even a plain append.The window now records that the starting snapshot is missing instead of raising:
_refresh_for_retryalready rebuilds on the refreshed head, which is also what Java'sSnapshotProducerdoes.CommitFailedExceptionwith a message saying the starting snapshot is not exposed, which is the pre-0.12 behavior and tells a lost race apart from a data conflict. They never continue without validation.Are these changes tested?
Yes. New tests in
tests/table/test_commit_retry.pyhide the starting snapshot from the refreshed metadata to emulate a catalog without history:test_append_retry_without_starting_snapshotraces two appends and asserts the loser retries and both land.test_delete_retry_without_starting_snapshot_raises_commit_failedasserts a delete raisesCommitFailedExceptioninstead ofValidationException.test_validate_concurrency_marks_missing_starting_snapshotcovers the window flags directly.All three fail on main and pass with the change. Full unit suite:
4570 passed, 5 skipped.prek run -apasses.Are there any user-facing changes?
Yes. On catalogs that only expose the current snapshot, retried appends now succeed instead of raising
ValidationException. Retried overwrites and deletes raiseCommitFailedExceptionagain, as they did before 0.12, with a message that names the missing starting snapshot.Closes #4327