laskoviymishka commented on code in PR #2071:
URL: https://github.com/apache/iceberg-go/pull/2071#discussion_r4153127762
##########
table/dv/deletion_vector_test.go:
##########
@@ -878,7 +879,124 @@ func TestReadDVInvalidPuffin(t *testing.T) {
offset, size := int64(4), int64(16)
_, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 0, &offset,
&size))
- assert.ErrorContains(t, err, "create puffin reader")
+ require.ErrorIs(t, err, ErrInvalidDeletionVector)
+ assert.ErrorContains(t, err, "not a Puffin container")
+}
+
+// Why: the bare-blob fallback must stay pinned to inputs that are genuinely
+// not Puffin. A file that starts with the Puffin magic but is truncated or has
+// a broken footer is a damaged Puffin file; routing it to the bare reader
would
+// hide a partial upload behind a "format mismatch" diagnostic.
+// Condition: valid header magic, footer missing or corrupt.
+// Assertion: ReadDV fails in the Puffin reader, and the error does not wrap
+// puffin.ErrNotPuffinFile.
+func TestReadDVTruncatedPuffinDoesNotFallBack(t *testing.T) {
+ dir := t.TempDir()
+ dvBlobBytes := readDVTestData(t,
"small-alternating-values-position-index.bin")
+ goodPath, meta := writePuffinWithDVBlob(t, dir, dvBlobBytes)
+ good, err := os.ReadFile(goodPath)
+ require.NoError(t, err)
+
+ cases := map[string][]byte{
Review Comment:
small thing: `cases` is a `map[string][]byte`, so the three subtests run in
random order (and `-shuffle` moves them again). No correctness impact since
each case writes its own file, but it makes `-v` output jump around versus the
slice-of-structs table the rest of the file uses. I'd switch it to `[]struct{
name string; raw []byte }` so the run order stays stable.
##########
table/dv/deletion_vector.go:
##########
@@ -327,6 +339,68 @@ func ReadDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap,
return bitmaps, nil
}
+// bareDVWarned records the bare (non-Puffin) DV files already reported, so a
+// snapshot with thousands of such files logs each path once.
+var bareDVWarned sync.Map
+
+// readBareDVs reads deletion vectors from an open file that is not a Puffin
+// container: the deletion-vector-v1 blobs are addressed directly by the
+// manifest's content_offset / content_size_in_bytes, exactly as the Java
+// reference reader (BaseDeleteLoader.readDV) does, which never consults the
+// Puffin footer. Databricks writes DVs for IcebergCompatV3 (UniForm) tables
+// this way: a Delta deletion_vector_*.bin file with a one-byte version
+// prefix and no Puffin header or footer.
+//
+// Without footer metadata the blob's type, referenced data file and
+// cardinality property cannot be cross-checked; the blob's own length,
+// magic and CRC-32 are still verified by DeserializeDV, and the decoded
+// cardinality is validated against the manifest record_count.
+//
+// dvFiles must already have passed validateDVFile (both callers validate
+// before dispatching here) and must all point at f. Blobs are read in
+// content_offset order to avoid backward seeks; results keep dvFiles order.
+func readBareDVs(f iceio.File, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap, error) {
+ filePath := dvFiles[0].FilePath()
+
+ order := make([]int, len(dvFiles))
+ for i := range order {
+ order[i] = i
+ }
+ offsetOf := func(i int) int64 {
+ _, _, _, contentOffset, _ :=
iceberginternal.BorrowedDataFilePointers(dvFiles[i])
+
+ return *contentOffset
+ }
+ slices.SortFunc(order, func(a, b int) int { return
cmp.Compare(offsetOf(a), offsetOf(b)) })
+
+ bitmaps := make([]*RoaringPositionBitmap, len(dvFiles))
+ for _, i := range order {
+ dvFile := dvFiles[i]
+ _, _, _, contentOffset, contentSize :=
iceberginternal.BorrowedDataFilePointers(dvFile)
+ offset, size := *contentOffset, *contentSize
+
+ data := make([]byte, size)
+ if _, err := f.ReadAt(data, offset); err != nil {
+ return nil, fmt.Errorf("%w: DV file %s is not a Puffin
container; direct read of %d bytes at offset %d: %w",
+ ErrInvalidDeletionVector, filePath, size,
offset, err)
+ }
+
+ bitmap, err := DeserializeDV(data, dvFile.Count())
+ if err != nil {
+ return nil, fmt.Errorf("%w: DV file %s is not a Puffin
container; blob at offset %d: %w",
+ ErrInvalidDeletionVector, filePath, offset, err)
+ }
+ bitmaps[i] = bitmap
+
+ if _, seen := bareDVWarned.LoadOrStore(filePath, struct{}{});
!seen {
Review Comment:
I'd hoist the `bareDVWarned` LoadOrStore + `slog.Warn` out of the read loop
to right after `filePath` is set, before the reads start. Reaching
`readBareDVs` at all already means we're committed to bare mode, so that's the
real boundary for the log, and it fixes the one case that bites: as written, if
the first blob's `DeserializeDV` fails we return before logging, so an operator
watching for that WARN line never learns we fell back on this file. It also
drops the per-blob re-probe, which today only takes effect on the first success
anyway.
--
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]
