fix(table): scope RollbackToSnapshot to the transaction's target branch - #1828
Conversation
Signed-off-by: badalprasadsingh <badal@datazip.io>
Signed-off-by: badalprasadsingh <badal@datazip.io>
zeroshade
left a comment
There was a problem hiding this comment.
Verified the fix end to end: branch resolution matches how Transaction.AssertRefSnapshotID resolves the default (t.branch, falling back to MainBranch), the CAS requirement is correctly built from the base metadata via baseRefSnapshotID rather than the staged builder, and the deliberate refusal to fall back to main for an unknown ref is exactly right — reusing currentSnapshotForRef here would have reintroduced #1827 in a worse form. The test coverage is genuinely thorough (divergent-lineage ancestry, tag rejection at both the operation and commit, requirement dedup with a producer, no-retry on type conflict).
Two things I explicitly checked and found fine:
- Replacing
meta.currentSnapshot()with ameta.refs[MainBranch]lookup for the default path is not a regression for legacy metadata that hascurrent-snapshot-idbut no explicitmainref:constructRefs/UnmarshalJSON synthesize the main ref fromcurrent-snapshot-id, so the ref is always present when a current snapshot exists (table/metadata.go:2202, 2282). - The skipped requirement when
baseRefSnapshotIDreturns nil (branch staged within this transaction) matches the existing pattern — the update that created the ref carries its own base-state requirement.
One message nit, non-blocking: the "branch %q does not exist" error at table/transaction.go:490 isn't wrapped in iceberg.ErrInvalidArgument while the tag-type error just below is; callers matching with errors.Is will only catch one of the two rejection paths. Fine to leave or fix in a follow-up.
LGTM.
Description
Fixes #1827
Resolves the branch the same way
AssertRefSnapshotIDalready does (t.branch, defaultmain), looks up that branch's own head from its own ref, and writes the ancestor check, the ref update, and the CAS assertion to it.Also made sure:
currentSnapshotForRef, from fix(table): resolve write parent snapshot from target branch head instead of main #1637, intentionally falls back to main for a new branch — reusing it here would let rolling back a nonexistent branch silently rewind main, reintroducing this exact bug.RollbackToSnapshotalways wrote toMainBranch, a branch by construction, so no type check was needed. Resolving an arbitrary named ref makes rewriting a tag as a branch (and dropping its tag retention) newly reachable.Testing
Added tests covering the relevant edge cases.