sevbanbayrak commented on code in PR #2071:
URL: https://github.com/apache/iceberg-go/pull/2071#discussion_r4145804668
##########
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:
Done in 12b915c: `NewReader` now checks the header magic first;
`ErrNotPuffinFile` only for a file shorter than 4 bytes or leading bytes ≠
`PFA1`. A valid-magic file that is too short keeps the plain `puffin: file too
small` error. The existing "file too small" case in `puffin_test.go` now uses
`PFA1tiny`, and a "too small for magic" case was added.
##########
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:
Done in 12b915c: `openDVReader` returns the still-open `iceio.File` with a
nil reader on `ErrNotPuffinFile`; `readBareDVs` takes that handle, so there is
a single `Open` per file and the callers keep one `defer f.Close()`.
##########
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:
Done in 12b915c: warning is deduplicated per file path (`bareDVWarned`
`sync.Map`) and emitted only after the first blob of that file has been read
and decoded.
##########
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:
Done in 12b915c: `readBareDVs` sorts by `content_offset` and restores input
order in the result; covered by the "blobs read in offset order, results in
input order" subtest.
##########
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:
Done in 12b915c: dropped the re-validation; `readBareDVs` is documented as
requiring pre-validated entries (both callers validate before dispatch).
--
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]
