laskoviymishka commented on code in PR #1221:
URL: https://github.com/apache/iceberg-go/pull/1221#discussion_r3441750714
##########
table/snapshot_producers.go:
##########
@@ -588,6 +590,33 @@ func (sp *snapshotProducer) removeDeleteFile(df
iceberg.DataFile) *snapshotProdu
return sp
}
+func (sp *snapshotProducer) removeDeletionVector(df iceberg.DataFile)
*snapshotProducer {
+ ref := df.ReferencedDataFile()
+ if ref == nil {
Review Comment:
`removeDeletionVector` silently drops the request when `ref == nil` and
returns `sp` unchanged. `ReplaceFiles` guards this upstream so normal flow
never reaches here with a nil ref — but any future direct caller would silently
lose the removal and orphan the exact DV this PR is fixing, with no error
signal.
I'd either return an error here or add a doc line stating callers must
guarantee a non-nil ref.
##########
table/transaction.go:
##########
@@ -932,6 +952,12 @@ 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")
}
+ // Relies on the spec invariant of at most one DV per referenced data
file:
Review Comment:
I think the comment has the failure mode backwards, and the check is unsound
in that case. `markedDVsForRemoval` is built by scanning `s.dataFiles`, while
`dvRefsToRemove` is a deduped map. If the table has the same DV ref in two
distinct manifest entries (a failed OCC retry that rewrote one manifest but
left the old one un-GC'd, or a corrupt table), the scan yields two entries for
one ref, so `len(markedDVsForRemoval)=2 > len(dvRefsToRemove)=1` — the check
*fails* and rejects a valid request. Duplicates don't "slip past" the check;
they trip it.
I'd dedupe `markedDVsForRemoval` by ref before comparing (or count distinct
refs found), and reword the comment to match. Otherwise compaction can
spuriously error on tables with duplicate DV manifest entries.
##########
table/scanner.go:
##########
@@ -179,8 +179,13 @@ func openManifest(io io.IO, manifest iceberg.ManifestFile,
return out, nil
}
+// isDeletionVector reports whether df is a deletion vector, keyed on the
Puffin
+// format to mirror Java's ContentFileUtil.isDV. Format is authoritative: a
+// spec-legal Parquet position-delete file may also set referenced_data_file,
so
+// keying on that would misclassify it. Callers that key removal by
+// referenced_data_file guard nil at each site, since a malformed DV may omit
it.
func isDeletionVector(df iceberg.DataFile) bool {
- return df.ReferencedDataFile() != nil
+ return df.FileFormat() == iceberg.PuffinFile
Review Comment:
I'd add a content-type guard here: `return df.FileFormat() ==
iceberg.PuffinFile && df.ContentType() == iceberg.EntryContentPosDeletes`.
The Java reference (`ContentFileUtil.isDV`) only checks `format() ==
PUFFIN`, but it's called with a `DeleteFile`, which is already restricted to
pos/eq-delete content. Our `isDeletionVector` takes an `iceberg.DataFile`,
which can carry any content type — so a Puffin file with some other content
type (a stats blob written as a delete-manifest entry by a buggy external
writer) would pass this and get routed as a DV during a scan. Our own
`DataFileBuilder` enforces `PuffinFile → EntryContentPosDeletes`, but external
writers aren't bound by that, so the guard is what makes the classifier safe on
the read side. wdyt?
##########
table/transaction.go:
##########
@@ -881,10 +881,23 @@ func (t *Transaction) ReplaceFiles(ctx context.Context,
dataFilesToDelete, dataF
}
setDeleteFilesToRemove := make(map[string]struct{},
len(deleteFilesToRemove))
+ dvRefsToRemove := make(map[string]struct{}, len(deleteFilesToRemove))
for i, df := range deleteFilesToRemove {
if df == nil {
return fmt.Errorf("nil delete file at index %d for
ReplaceFiles", i)
}
+ if isDeletionVector(df) {
Review Comment:
The DV branch `continue`s before the `path == ""` guard runs, so a DV with a
non-nil ref but an empty `FilePath()` slips past all validation and lands in
the snapshot as a delete-manifest entry with an empty path string — malformed
metadata that confuses future readers.
The in-process flow never hits this since `CollectSafeDeletionVectors`
returns live scan-planning objects with real paths, but `ReplaceFiles` is
public and a serialization round-trip through `ApplyResult` could drop the path
field. I'd add the same non-empty check inside this block before the `continue`:
```go
if df.FilePath() == "" {
return errors.New("deletion vector paths must be non-empty for
ReplaceFiles")
}
```
##########
table/rewrite_data_files.go:
##########
@@ -524,3 +537,31 @@ func CollectSafePositionDeletes(tasks []FileScanTask)
[]iceberg.DataFile {
return safe
}
+
+// CollectSafeDeletionVectors returns the deletion vectors attached to the
+// tasks, deduplicated by referenced data file.
+//
+// Safety rests on the scan-planning invariant that a task carries only the DV
+// referencing its own data file (matchDVToData keys on task.File's path), so
+// every DV returned references a file in the rewrite set. A caller that
+// hand-builds a [FileScanTask] with a DV whose ReferencedDataFile is some
other
+// live data file would have that DV marked safe to expunge here, silently
+// resurrecting its deleted rows — populate DeletionVectorFiles only from scan
+// planning.
+func CollectSafeDeletionVectors(tasks []FileScanTask) []iceberg.DataFile {
Review Comment:
Minor, but `map[string]bool` as a sentinel set is a lint target —
`map[string]struct{}` is the idiomatic form. `CollectSafePositionDeletes` right
above has the same pattern; while we're here I'd switch both. Not blocking.
##########
table/dv_rewrite_test.go:
##########
@@ -0,0 +1,300 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table_test
+
+import (
+ "context"
+ "path/filepath"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ iceio "github.com/apache/iceberg-go/io"
+ "github.com/apache/iceberg-go/table"
+ "github.com/apache/iceberg-go/table/dv"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// TestRewriteDataFilesRemovesDeletionVectors guarantees that compacting a data
+// file carrying a deletion vector expunges that DV in the same rewrite
snapshot,
+// leaving no manifest entry referencing the removed data file. Both commit
+// paths of [Transaction.RewriteDataFiles] are covered: the single atomic
+// snapshot and the per-group partial-progress path.
+//
+// Background: a DV manifest entry is 1:1 with its referenced data file but
rides
+// in FileScanTask.DeletionVectorFiles, separate from the pos-delete files in
+// FileScanTask.DeleteFiles. A rewrite that collected only the latter would
leave
+// the DV behind, referencing a data file no longer in the table — an orphan.
+//
+// An orphaned DV is invisible to scan planning — matchDVToData drops a DV
whose
+// referenced data file is gone — so the assertion walks the raw delete
manifests
+// of the post-compaction snapshot.
+func TestRewriteDataFilesRemovesDeletionVectors(t *testing.T) {
+ for _, partialProgress := range []bool{false, true} {
+ name := "atomic"
+ if partialProgress {
+ name = "partial-progress"
+ }
+ t.Run(name, func(t *testing.T) {
+ ctx := context.Background()
+ tbl, dvTarget := seedV3TableWithDV(t)
+
+ groups, rewritten := planDVCompaction(t, tbl, dvTarget)
+
+ rtx := tbl.NewTransaction()
+ _, err := rtx.RewriteDataFiles(ctx, groups,
+ table.RewriteDataFilesOptions{PartialProgress:
partialProgress})
+ require.NoError(t, err)
+ tbl, err = rtx.Commit(ctx)
+ require.NoError(t, err)
+
+ assertRowCount(t, tbl, 3) // DV applied during the
rewrite; id=1 stays gone
+ assert.Empty(t, deleteEntriesReferencing(t, tbl,
rewritten),
+ "compaction must remove deletion vectors for
rewritten data files")
+ })
+ }
+}
+
+// TestRewriteFilesApplyResultRemovesDeletionVectors drives the distributed
+// coordinator path — a worker runs [ExecuteCompactionGroup], then a leader
+// stages the results with [Transaction.NewRewrite] +
[RewriteFiles.ApplyResult]
+// + Commit. It locks the ApplyResult→ReplaceFiles coupling that carries
+// SafeDeletionVectors and expunges them by referenced data file.
+func TestRewriteFilesApplyResultRemovesDeletionVectors(t *testing.T) {
+ ctx := context.Background()
+ tbl, dvTarget := seedV3TableWithDV(t)
+
+ groups, rewritten := planDVCompaction(t, tbl, dvTarget)
+
+ var results []table.CompactionGroupResult
+ dvCount := 0
+ for _, g := range groups {
+ gr, err := table.ExecuteCompactionGroup(ctx, tbl, g)
+ require.NoError(t, err)
+ results = append(results, gr)
+ dvCount += len(gr.SafeDeletionVectors)
+ }
+ require.Positive(t, dvCount, "worker must collect the DV for removal")
Review Comment:
The tests cover the happy paths well, but none of the error paths
`ReplaceFiles` now introduces are exercised — the missing-ref error, the
distinct-refs error, and the "DV not in table" count check. Given those
branches are where the soundness questions live, I'd add at least one test that
drives a DV-not-in-table removal and one for the duplicate-ref case once that
check is settled. wdyt?
--
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]
