zeroshade commented on code in PR #2239:
URL: https://github.com/apache/iceberg-go/pull/2239#discussion_r4328525235
##########
table/transaction.go:
##########
@@ -2184,6 +2212,9 @@ func (t *Transaction) replaceFiles(ctx context.Context,
dataFilesToDelete, dataF
for _, df := range markedDVsForRemoval {
updater.removeDeletionVector(df)
}
+ for _, df := range danglingDVs {
Review Comment:
Worth a changelog line: `removeDeletionVector` makes the commit
non-replayable, so v3 compaction (`RewriteFiles`/`ReplaceFiles`) over files
with DVs now fails on a CAS conflict instead of rebuilding and retrying, and
callers need to re-plan. That's the right trade over leaving dangling DVs, but
it's a visible behavior change for compaction jobs.
##########
table/transaction.go:
##########
@@ -1732,6 +1745,15 @@ func (t *Transaction) ReplaceDataFilesWithDataFiles(ctx
context.Context, filesTo
return errors.New("cannot delete files that do not belong to
the table")
}
+ // On v3 every removed file takes its deletion vector with it.
+ // DVs only exist from v3, so v2 skips the read.
+ var dvs liveDeletionVectors
+ if meta.formatVersion >= 3 {
Review Comment:
This reads every live DV on v3 even when `markedForDeletion` is empty.
`ReplaceDataFiles` guards the same read with `len(markedForDeletion) > 0` (line
1012); the same guard here would skip the full delete-manifest scan (#2160)
when nothing is removed.
##########
table/transaction.go:
##########
@@ -1985,6 +2009,10 @@ func (t *Transaction) replaceFiles(ctx context.Context,
dataFilesToDelete, dataF
}
} else if want, ok :=
autoDVsToRemoveByRef[*ref]; ok && want.FilePath() == path {
markedDVsForRemoval[*ref] = df
+ } else if _, ok := setToDelete[*ref]; ok {
+ // The spec requires a removed data
file to take its DV with it, named or not.
+ // Like automatic removals, it does not
set the sequence number of added delete files.
+ danglingDVs[*ref] = df
Review Comment:
This ordering looks right to me: it only fires after the named
(`dvsToRemoveByRef`) and auto (`autoDVsToRemoveByRef`) checks miss on data file
plus path. So a caller who names a stale DV still hits the existing "not found"
error, and this branch removes the live one, which is correct since its data
file is going away. Keeping it out of `removedDeleteSequenceNumbers` matches
automatic removals.
--
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]
