zeroshade commented on code in PR #1637:
URL: https://github.com/apache/iceberg-go/pull/1637#discussion_r3732113603
##########
table/table.go:
##########
@@ -689,12 +703,11 @@ func rewriteRefSnapshotRequirements(reqs []Requirement,
branch string, fresh Met
// list is rewritten to include the fresh parent's manifests so that the
// rebuilt snapshot contains every committed file.
func rebuildSnapshotUpdates(ctx context.Context, updates []Update, freshMeta
Metadata, branch string, fs icebergio.WriteFileIO, attempt int) (rebuilt
[]Update, orphanedPaths []string, err error) {
- // Determine the fresh branch head to use as the rebuilt snapshot's
parent.
+ // Must mirror createSnapshotProducer's attempt-0 fallback, or a
retried new
+ // branch rebuilds with a nil parent and silently drops main's data.
var freshHead *Snapshot
- if branch != "" && freshMeta != nil {
- freshHead = freshMeta.SnapshotByName(branch)
- } else if freshMeta != nil {
- freshHead = freshMeta.CurrentSnapshot()
+ if freshMeta != nil {
+ freshHead = latestSnapshotForBranch(freshMeta, branch)
Review Comment:
`freshHead` is calculated once and then supplied to every staged
`addSnapshotUpdate`. If a transaction stages snapshots A and B and retries,
both are rebuilt as children of the same branch head; they become siblings, and
the final ref to B omits A. That is a silent-data-loss path.
Suggested fix: replay staged snapshot updates sequentially, making each
rebuilt snapshot the logical parent of the next one, and add a forced-retry
test with at least two staged snapshots.
##########
table/metadata.go:
##########
@@ -390,6 +390,45 @@ func (b *MetadataBuilder) currentSnapshot() *Snapshot {
return s
}
+// currentSnapshotForRef resolves the parent for createSnapshotProducer and
mergeOverwrite.
+// An unknown branch falls back to currentSnapshot() so a new
+// branch forks from main — the opposite of currentSnapshotIDForRef,
+// which must return nil there so AssertRefSnapshotID can prove the branch is
absent;
+// never derive one from the other. A present-but-dangling ref also yields nil.
+func (b *MetadataBuilder) currentSnapshotForRef(ref string) *Snapshot {
+ if ref == "" || ref == MainBranch {
+ return b.currentSnapshot()
+ }
+
+ r, ok := b.refs[ref]
Review Comment:
This accepts any named ref without checking its type. A transaction
targeting a tag will resolve the tag as its parent, then `commitManifests`
emits a `BranchRef` replacement: the write advances the tag and converts it
into a branch. The same bug exists if an initially absent branch name becomes a
tag during retry.
Suggested fix: reject non-branch refs both when constructing the transaction
and after every metadata refresh before replay/commit, with tests for an
existing tag and the absent-name→tag race.
##########
table/metadata.go:
##########
@@ -390,6 +390,45 @@ func (b *MetadataBuilder) currentSnapshot() *Snapshot {
return s
}
+// currentSnapshotForRef resolves the parent for createSnapshotProducer and
mergeOverwrite.
+// An unknown branch falls back to currentSnapshot() so a new
+// branch forks from main — the opposite of currentSnapshotIDForRef,
+// which must return nil there so AssertRefSnapshotID can prove the branch is
absent;
+// never derive one from the other. A present-but-dangling ref also yields nil.
+func (b *MetadataBuilder) currentSnapshotForRef(ref string) *Snapshot {
+ if ref == "" || ref == MainBranch {
+ return b.currentSnapshot()
+ }
+
+ r, ok := b.refs[ref]
+ if !ok {
+ return b.currentSnapshot()
+ }
+
+ s, _ := b.SnapshotByID(r.SnapshotID)
Review Comment:
A present ref whose snapshot cannot be resolved is silently treated as
parentless here. Although loaded metadata should already reject this state,
this helper should fail closed rather than turning a dangling ref into a
first-snapshot write.
Suggested fix: make the lookup fallible (or validate before reaching it) and
propagate `ErrInvalidMetadata` when a named ref points to a missing snapshot.
--
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]
