laskoviymishka commented on code in PR #1221:
URL: https://github.com/apache/iceberg-go/pull/1221#discussion_r3430057186
##########
table/snapshot_producers.go:
##########
@@ -588,6 +590,34 @@ func (sp *snapshotProducer) removeDeleteFile(df
iceberg.DataFile) *snapshotProdu
return sp
}
+func (sp *snapshotProducer) removeDeletionVector(df iceberg.DataFile)
*snapshotProducer {
+ sp.deletedDVsByRef[*df.ReferencedDataFile()] = df
Review Comment:
This dereferences `ReferencedDataFile()` with nothing guarding nil — it
works today only because every caller pre-filters through
`isDeletionVectorFile`. That precondition isn't documented or enforced here,
and the analogous `removeDeleteFile` doesn't deref a pointer at all, so there's
no precedent for the unsafe contract.
I'd add a guard at the top so a future caller wired in without the pre-check
fails closed rather than panicking:
```go
ref := df.ReferencedDataFile()
if ref == nil {
return sp
}
sp.deletedDVsByRef[*ref] = df
```
Fix this and the snapshot side is good to land.
##########
table/rewrite_data_files.go:
##########
@@ -430,7 +441,8 @@ func (t *Transaction) rewriteDataFilesPartial(ctx
context.Context, groups []Comp
continue
}
- if err := t.ReplaceFiles(ctx, gr.OldDataFiles, gr.NewDataFiles,
gr.SafePosDeletes,
+ deletesToRemove := append(slices.Clone(gr.SafePosDeletes),
gr.SafeDeletionVectors...)
Review Comment:
Both tests run with the default `PartialProgress: false`, so this branch —
concatenating `SafeDeletionVectors` into the `ReplaceFiles` input — never runs
in CI, and it's the path operators are most likely to hit on large jobs (a
checkpoint per group).
I'd parameterize one of the existing DV tests over `PartialProgress: true`,
or add a sibling case, asserting the same orphan-absence. While we're at it, a
test that drives the coordinator pattern directly — `tx.NewRewrite(nil)` →
`ApplyResult(gr)` for a group with DVs → `Commit` → assert no orphaned DV entry
— would lock the `ApplyResult`/`ReplaceFiles` coupling from the comment on
`rewrite_files.go`. Both paths the PR targets would then be covered.
##########
table/rewrite_data_files.go:
##########
@@ -524,3 +537,25 @@ func CollectSafePositionDeletes(tasks []FileScanTask)
[]iceberg.DataFile {
return safe
}
+
+// CollectSafeDeletionVectors returns the deletion vectors attached to the
+// tasks. A DV is bound 1:1 to the data file it references, and that file is
+// always in the rewrite set (it is the task's own file), so every DV here is
+// safe to expunge. Deduplicated by referenced data file.
+func CollectSafeDeletionVectors(tasks []FileScanTask) []iceberg.DataFile {
Review Comment:
The "always in the rewrite set" claim is correct, but only because scan
planning matches a DV to exactly its own task's file — and that invariant isn't
stated. `CollectSafePositionDeletes` carries a full caller-contract warning
right above it; this one just asserts the conclusion.
The gap bites a caller that hand-constructs tasks (a test helper, a
non-standard coordinator): if a `FileScanTask` has a `DeletionVectorFiles[i]`
whose `ReferencedDataFile()` isn't `task.File.FilePath()`, we'd collect a DV
pointing at a live data file and mark it safe to expunge — silently
resurrecting deleted rows in that other file.
I'd mirror the `CollectSafePositionDeletes` contract note here, spelling out
that safety relies on the scan-planning invariant that each task owns the DVs
it carries.
##########
table/rewrite_files.go:
##########
@@ -189,7 +189,12 @@ func (r *RewriteFiles) Apply(deletes, adds, safeDeletes
[]iceberg.DataFile) *Rew
// }
// if err := rewrite.Commit(ctx); err != nil { ... }
func (r *RewriteFiles) ApplyResult(gr CompactionGroupResult) *RewriteFiles {
- return r.Apply(gr.OldDataFiles, gr.NewDataFiles, gr.SafePosDeletes)
+ r.Apply(gr.OldDataFiles, gr.NewDataFiles, gr.SafePosDeletes)
+ for _, dv := range gr.SafeDeletionVectors {
Review Comment:
This works, but the correctness is invisible: a DV routed through
`DeleteFile` lands in `deleteFilesToRemove` (because its `ContentType` is
`EntryContentPosDeletes`), and `ReplaceFiles` then re-identifies it as a DV via
`isDeletionVectorFile` to split it back out. That coupling breaks non-obviously
if someone teaches `DeleteFile` to reject Puffin files or refactors the
splitting in `ReplaceFiles`. A one-line comment here saying why
`DeleteFile(dv)` is the right channel would save the next reader the trace.
The other half: `Apply` is still public and undeprecated, and a coordinator
calling `rewriteFiles.Apply(...)` directly still orphans DVs exactly as before
— its signature can't carry `SafeDeletionVectors`. The doc now says
coordinators "must use ApplyResult" but nothing enforces it. I'd add `//
Deprecated: use ApplyResult, which carries SafeDeletionVectors.` to `Apply` so
the compiler nudges existing callers over.
##########
table/snapshot_producers.go:
##########
@@ -588,6 +590,34 @@ func (sp *snapshotProducer) removeDeleteFile(df
iceberg.DataFile) *snapshotProdu
return sp
}
+func (sp *snapshotProducer) removeDeletionVector(df iceberg.DataFile)
*snapshotProducer {
+ sp.deletedDVsByRef[*df.ReferencedDataFile()] = df
+
+ return sp
+}
+
+// deleteFileRemoved reports whether a delete-file entry is being expunged in
+// this snapshot: position/equality deletes match by path, deletion vectors by
+// referenced data file (one Puffin holds blobs for several data files, so a
+// shared path would over-remove live siblings).
+func (sp *snapshotProducer) deleteFileRemoved(df iceberg.DataFile) bool {
+ if _, ok := sp.deletedDeleteFiles[df.FilePath()]; ok {
+ return true
+ }
+ if isDeletionVectorFile(df) {
+ _, ok := sp.deletedDVsByRef[*df.ReferencedDataFile()]
+
+ return ok
+ }
+
+ return false
Review Comment:
We now have two near-identical predicates in the `table` package: this one,
and `isDeletionVector` in `scanner.go:182`, which checks only
`ReferencedDataFile() != nil`. Same concept, different semantics, confusingly
similar names.
The divergence isn't harmless — Java's `ContentFileUtil.isDV` keys purely on
`format() == PUFFIN`, and the spec permits file-scoped Parquet pos-delete files
that set `referenced_data_file`. The `scanner.go` version would misclassify one
of those as a DV and route it to `DeletionVectorFiles` instead of
`DeleteFiles`. This new one is closer to correct.
I'd consolidate to a single predicate keyed on the Puffin format check and
use it in both places. wdyt?
##########
table/transaction.go:
##########
@@ -932,6 +949,9 @@ func (t *Transaction) ReplaceFiles(ctx context.Context,
dataFilesToDelete, dataF
if len(markedDeleteForRemoval) != len(setDeleteFilesToRemove) {
return errors.New("cannot remove delete files that do not
belong to the table")
}
+ if len(markedDVsForRemoval) != len(dvRefsToRemove) {
Review Comment:
This count check is fine for a well-formed table, but it leans on the same
unstated assumption as the sibling data/pos-delete checks: that no
`referenced_data_file` appears twice. If a broken table had two entries for the
same ref, both would append to `markedDVsForRemoval` while the `dvRefsToRemove`
map dedups, so `!=` wouldn't catch the over-count and one entry would survive.
Not worth a guard, but a one-line comment noting the distinct-ref invariant
this relies on would help.
--
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]
