laskoviymishka commented on code in PR #2071:
URL: https://github.com/apache/iceberg-go/pull/2071#discussion_r4145282387
##########
table/dv/deletion_vector.go:
##########
@@ -327,6 +338,54 @@ func ReadDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap,
return bitmaps, nil
}
+// readBareDVs reads deletion vectors from a 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.
+func readBareDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap, error) {
+ filePath := dvFiles[0].FilePath()
+ f, err := fs.Open(filePath)
+ if err != nil {
+ return nil, fmt.Errorf("open DV file %s: %w", filePath, err)
+ }
+ defer f.Close()
+
+ slog.Warn("DV file is not a Puffin container; reading
deletion-vector-v1 blobs directly at content_offset, footer metadata validation
skipped",
+ "dv_file", filePath)
+
+ bitmaps := make([]*RoaringPositionBitmap, len(dvFiles))
+ for i, dvFile := range dvFiles {
Review Comment:
The Puffin `ReadDVs` path sorts blob reads by offset and coalesces adjacent
ones to cut round-trips; this loop reads in input order, so multiple blobs in
the same file can cause backward seeks, each its own range request. Not
blocking, but sorting `dvFiles` by `*contentOffset` here (restoring output
order after) would keep the two paths consistent.
##########
table/dv/deletion_vector.go:
##########
@@ -327,6 +338,54 @@ func ReadDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap,
return bitmaps, nil
}
+// readBareDVs reads deletion vectors from a 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.
+func readBareDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap, error) {
+ filePath := dvFiles[0].FilePath()
+ f, err := fs.Open(filePath)
Review Comment:
`openDVReader` already opened this file, read the header, and closed it
before returning `ErrNotPuffinFile`, so this is a second `Open` on the same
path. On object storage that's an extra round-trip per DV file, which adds up
on the large UniForm tables this PR is aimed at. I'd have `openDVReader` hand
back the still-open `iceio.File` on the not-Puffin case (or peek the first 4
magic bytes in `ReadDV`/`ReadDVs` before dispatch) so the fallback reuses the
handle instead of reopening.
##########
table/dv/deletion_vector_test.go:
##########
@@ -878,7 +878,66 @@ 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")
Review Comment:
`TestReadDVInvalidPuffin` now feeds a 17-byte file, which is the too-small
to bare-path case, so nothing here pins the tighter invariant: a file that
starts with real Puffin magic but has a broken/truncated footer should fail in
`readFooter` with an error that does NOT wrap `ErrNotPuffinFile`, i.e. the bare
fallback must not activate. I'd add that case so a future change that
accidentally wraps footer errors in `ErrNotPuffinFile` can't silently route a
corrupt Puffin file to the bare reader.
##########
table/dv/deletion_vector.go:
##########
@@ -327,6 +338,54 @@ func ReadDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap,
return bitmaps, nil
}
+// readBareDVs reads deletion vectors from a 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.
+func readBareDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap, error) {
+ filePath := dvFiles[0].FilePath()
+ f, err := fs.Open(filePath)
+ if err != nil {
+ return nil, fmt.Errorf("open DV file %s: %w", filePath, err)
+ }
+ defer f.Close()
+
+ slog.Warn("DV file is not a Puffin container; reading
deletion-vector-v1 blobs directly at content_offset, footer metadata validation
skipped",
+ "dv_file", filePath)
+
+ bitmaps := make([]*RoaringPositionBitmap, len(dvFiles))
+ for i, dvFile := range dvFiles {
+ if err := validateDVFile(dvFile); err != nil {
Review Comment:
Both `ReadDV` and `ReadDVs` already `validateDVFile` before dispatching
here, so this re-validates (and re-runs `BorrowedDataFilePointers`) once per
entry. Harmless but redundant. I'd either drop it here and document that
`readBareDVs` requires pre-validated entries, or keep it and note the
redundancy is intentional so the function stays safe to call standalone.
##########
table/dv/deletion_vector.go:
##########
@@ -327,6 +338,54 @@ func ReadDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap,
return bitmaps, nil
}
+// readBareDVs reads deletion vectors from a 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.
+func readBareDVs(fs iceio.IO, dvFiles []iceberg.DataFile)
([]*RoaringPositionBitmap, error) {
+ filePath := dvFiles[0].FilePath()
+ f, err := fs.Open(filePath)
+ if err != nil {
+ return nil, fmt.Errorf("open DV file %s: %w", filePath, err)
+ }
+ defer f.Close()
+
+ slog.Warn("DV file is not a Puffin container; reading
deletion-vector-v1 blobs directly at content_offset, footer metadata validation
skipped",
Review Comment:
This fires once per `readBareDVs` call, and since `ReadDV` calls it once per
file, a scan over a snapshot with thousands of bare DV files emits thousands of
identical WARN lines. Useful once, noise after that. I'd dedup per distinct
file path, or move it to the public API boundary.
While we're here: it also fires before the per-entry `validateDVFile` in the
loop, so a caller that skips pre-validation would log "reads are happening" and
then immediately fail validation. Emitting after the first entry validates
would keep the message honest.
##########
puffin/puffin_reader.go:
##########
@@ -109,7 +115,7 @@ func NewReader(r ReaderAtSeeker, opts ...ReaderOption)
(*Reader, error) {
// [Magic] + zero for blob + [Magic] + [FooterPayloadSize (assuming
~0)] + [Flags] + [Magic]
minSize := int64(MagicSize + MagicSize + footerTrailerSize)
if size < minSize {
- return nil, fmt.Errorf("puffin: file too small (%d bytes,
minimum %d)", size, minSize)
+ return nil, fmt.Errorf("%w: file too small (%d bytes, minimum
%d)", ErrNotPuffinFile, size, minSize)
Review Comment:
Folding the too-small case into `ErrNotPuffinFile` means a truncated or
half-written real Puffin DV file (under 20 bytes) now routes to the bare-blob
path and surfaces as `ErrInvalidDeletionVector: ... not a Puffin container ...
direct read ...`, which points the operator at a format mismatch when the real
cause is a partial upload or storage failure. Corruption still gets caught, so
this isn't a data bug, just a misleading diagnostic.
The invalid-magic branch below is genuinely not-Puffin and belongs under
this sentinel. The size branch doesn't: I'd check the header magic first and
only return `ErrNotPuffinFile` for the too-small case when the file is under 4
bytes (can't confirm magic) or the leading bytes aren't `PFA1`. A `>= 4`-byte
file that starts with Puffin magic but is too short is a truncated Puffin file,
and should keep the old `puffin: file too small` error rather than fall through
to the bare reader.
##########
table/dv/deletion_vector_test.go:
##########
@@ -878,7 +878,66 @@ 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: Databricks writes deletion vectors for IcebergCompatV3 (UniForm) tables
+// as a Delta deletion_vector_*.bin — a one-byte version prefix followed by
+// deletion-vector-v1 blobs, with no Puffin header or footer — and the manifest
+// addresses the blob with content_offset / content_size_in_bytes. The Java
+// reference reader reads these directly; so must we.
+// Condition: the DV file has no Puffin container but the manifest range holds
a valid blob.
+// Assertion: ReadDV/ReadDVs decode the blob and validate cardinality against
record_count.
+func TestReadDVBareBlobWithoutPuffinContainer(t *testing.T) {
+ dir := t.TempDir()
+ path := filepath.Join(dir, "deletion_vector_0001.bin")
+
+ first := NewRoaringPositionBitmap()
+ first.Set(1)
+ first.Set(9)
+ firstData, err := SerializeDV(first)
+ require.NoError(t, err)
+ second := NewRoaringPositionBitmap()
+ second.Set(7)
+ secondData, err := SerializeDV(second)
+ require.NoError(t, err)
+
+ raw := append([]byte{0x01}, firstData...) // Delta DV file version
byte, then blobs back to back
+ secondOffset := int64(len(raw))
+ raw = append(raw, secondData...)
+ require.NoError(t, os.WriteFile(path, raw, 0o644))
+
+ firstOffset, firstSize := int64(1), int64(len(firstData))
+ bm, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 2, &firstOffset,
&firstSize))
+ require.NoError(t, err)
+ assert.Equal(t, int64(2), bm.Cardinality())
+ assert.True(t, bm.Contains(1))
+ assert.True(t, bm.Contains(9))
+
+ secondSize := int64(len(secondData))
+ files := []iceberg.DataFile{
+ newDVTestFile(path, 2, &firstOffset, &firstSize),
+ newDVTestFile(path, 1, &secondOffset, &secondSize),
+ }
+ bitmaps, err := ReadDVs(iceio.LocalFS{}, files)
+ require.NoError(t, err)
+ require.Len(t, bitmaps, 2)
+ assert.Equal(t, int64(2), bitmaps[0].Cardinality())
+ assert.True(t, bitmaps[1].Contains(7))
+
+ t.Run("cardinality still validated against record_count", func(t
*testing.T) {
+ _, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 3,
&firstOffset, &firstSize))
+ require.ErrorIs(t, err, ErrInvalidDeletionVector)
+ assert.ErrorContains(t, err, "cardinality mismatch")
+ })
+
+ t.Run("range beyond file", func(t *testing.T) {
+ badOffset, badSize := int64(1), int64(len(raw)+8)
+ _, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 2,
&badOffset, &badSize))
+ require.ErrorIs(t, err, ErrInvalidDeletionVector)
+ assert.ErrorContains(t, err, "direct read")
Review Comment:
The subtests cover ReadAt failing (range beyond file) and post-deserialize
cardinality mismatch, but not the in-between: ReadAt succeeds and
`DeserializeDV` rejects the blob for a DV-level reason (bad magic / CRC), which
is the `blob at offset` wrap in `readBareDVs`. I'd add a subtest that flips the
last 4 CRC bytes and asserts `ErrorIs(err, ErrInvalidDeletionVector)` plus the
"blob at offset" message.
--
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]
