laskoviymishka commented on code in PR #1615:
URL: https://github.com/apache/iceberg-go/pull/1615#discussion_r3719181196
##########
table/dv/deletion_vector.go:
##########
@@ -176,9 +183,23 @@ func ReadDV(fs iceio.IO, dvFile iceberg.DataFile)
(*RoaringPositionBitmap, error
}
offset := *dvFile.ContentOffset()
- blobData := make([]byte, size)
- if _, err := reader.ReadAt(blobData, offset); err != nil {
- return nil, fmt.Errorf("read DV blob at offset %d: %w", offset,
err)
+ blob, err := findBlobMetadataByRange(reader.Blobs(), offset, size)
Review Comment:
Worth a couple of sentences in the `ReadDV` doc about how deliberate this
strictness is.
Java's `DVUtil.readDV` reads purely by `contentOffset`/`contentSizeInBytes`
and validates neither the blob type nor `referenced-data-file`; PyIceberg does
the same. So after this we reject spec-non-conformant third-party Puffin files
that both of them would happily read. I think that's the right call, since both
properties are spec-required and our own writer always sets them, but it's a
real divergence and the next person to hit it should find the reasoning in the
doc rather than in git history.
The asymmetry deserves a line too: a missing `referenced-data-file` is fatal
while a missing `cardinality` is only a warning, and both are spec-required.
Identity check versus redundancy check is a sound distinction, it just isn't
visible from the code.
##########
table/dv/deletion_vector_test.go:
##########
@@ -139,28 +139,31 @@ func writePuffinWithDVBlobAndProps(t *testing.T, dir
string, dvBlobBytes []byte,
// writer) is to assemble the bytes directly. The reader's validateBlobs does
// not require the property, so this file loads cleanly.
func writeRawPuffinWithDVBlobNoCardinality(t *testing.T, dir string,
dvBlobBytes []byte) (string, puffin.BlobMetadata) {
+ return writeRawPuffinBlob(t, dir, "raw-dv-no-cardinality.puffin",
dvBlobBytes, puffin.BlobTypeDeletionVector, map[string]string{
+ "referenced-data-file": "s3://bucket/data/data-001.parquet",
Review Comment:
`dvReferencedDataFileProperty` is visible here (we're in package `dv`, not
`dv_test`), so I'd use the constant rather than the literal. Same for the
`"cardinality"` keys in the other fixtures while we're here. As written,
renaming the constant compiles clean and the fixtures quietly stop matching
what the reader looks up.
##########
table/dv/deletion_vector.go:
##########
@@ -176,9 +183,23 @@ func ReadDV(fs iceio.IO, dvFile iceberg.DataFile)
(*RoaringPositionBitmap, error
}
offset := *dvFile.ContentOffset()
- blobData := make([]byte, size)
- if _, err := reader.ReadAt(blobData, offset); err != nil {
- return nil, fmt.Errorf("read DV blob at offset %d: %w", offset,
err)
+ blob, err := findBlobMetadataByRange(reader.Blobs(), offset, size)
+ if err != nil {
+ return nil, fmt.Errorf("DV file %s: %w", dvFile.FilePath(), err)
+ }
+ if blob.Type != puffin.BlobTypeDeletionVector {
+ return nil, fmt.Errorf("DV file %s: blob at offset %d has type
%q, expected %q",
+ dvFile.FilePath(), offset, blob.Type,
puffin.BlobTypeDeletionVector)
+ }
+
+ referencedDataFile, ok := blob.Properties[dvReferencedDataFileProperty]
+ if !ok || referencedDataFile == "" {
+ return nil, fmt.Errorf("DV file %s: blob at offset %d missing
%s property",
Review Comment:
When the property is present but empty we report it as "missing", which
points an operator diagnosing a corrupt blob at the wrong thing. I'd make it
`missing or empty %s property`, or split the two conditions if we want them
separately diagnosable. Either is fine, just not "missing" for a key that's
there.
##########
table/dv/deletion_vector.go:
##########
@@ -211,23 +232,20 @@ func ReadDV(fs iceio.IO, dvFile iceberg.DataFile)
(*RoaringPositionBitmap, error
"dv_file", dvFile.FilePath(), "offset", offset)
}
+ blobData := make([]byte, size)
+ if _, err := reader.ReadAt(blobData, offset); err != nil {
+ return nil, fmt.Errorf("read DV blob at offset %d: %w", offset,
err)
+ }
+
// Validate the decoded bitmap against the manifest record_count (always
// present, including zero). When the puffin property is present it has
// already been confirmed to agree with this value above.
return DeserializeDV(blobData, manifestCardinality)
}
-// blobCardinality returns the cardinality declared by the puffin blob at the
-// manifest entry's (offset, size). The bool indicates whether the property was
-// present:
-//
-// - (n, true, nil) — property found and parsed successfully
-// - (0, false, nil) — matching blob found but no cardinality property
-// - (_, _, err) — manifest/footer mismatch or property unparseable
-//
-// Keeping the sentinel out of the int64 return channel avoids leaking
-// DeserializeDV's "-1 means skip" convention up the call chain.
-func blobCardinality(blobs []puffin.BlobMetadata, offset, size int64) (int64,
bool, error) {
+// findBlobMetadataByRange returns the footer entry identified by the
+// manifest's content offset and size.
+func findBlobMetadataByRange(blobs []puffin.BlobMetadata, offset, size int64)
(puffin.BlobMetadata, error) {
Review Comment:
Worth noting in the doc that the returned `puffin.BlobMetadata` is a zero
value on the error paths and must not be read. The caller is correct today (the
`err` check precedes `blob.Type`), but returning `*puffin.BlobMetadata` with
nil on error would make that self-documenting instead of conventional. Either
works, wdyt?
##########
table/dv/deletion_vector_test.go:
##########
@@ -425,9 +450,55 @@ func TestReadDVInvalidPuffin(t *testing.T) {
assert.ErrorContains(t, err, "create puffin reader")
}
+// Why: offset, size, and cardinality cannot prove that the selected Puffin
blob
+// is a deletion vector for the manifest's referenced data file.
+// Condition: the matched blob has conflicting, missing, or non-DV identity
metadata.
+// Assertion: ReadDV rejects each case before decoding the blob payload.
+func TestReadDVValidatesBlobMetadata(t *testing.T) {
+ dvBlobBytes := readDVTestData(t,
"small-alternating-values-position-index.bin")
+
+ t.Run("mismatched referenced data file", func(t *testing.T) {
+ dir := t.TempDir()
+ path, meta := writePuffinWithDVBlobAndProps(t, dir,
dvBlobBytes, map[string]string{
+ "referenced-data-file":
"s3://bucket/data/data-002.parquet",
+ "cardinality": "5",
+ })
+ offset, size := meta.Offset, meta.Length
+
+ _, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 5,
&offset, &size))
+ require.Error(t, err)
+ assert.ErrorContains(t, err, "manifest referenced_data_file")
+ assert.ErrorContains(t, err, "data-001.parquet")
+ assert.ErrorContains(t, err, "data-002.parquet")
+ })
+
+ t.Run("missing puffin referenced data file", func(t *testing.T) {
+ dir := t.TempDir()
+ path, meta := writeRawPuffinBlob(t, dir,
"raw-dv-no-reference.puffin", dvBlobBytes,
+ puffin.BlobTypeDeletionVector,
map[string]string{"cardinality": "5"})
+ offset, size := meta.Offset, meta.Length
+
+ _, err := ReadDV(iceio.LocalFS{}, newDVTestFile(path, 5,
&offset, &size))
+ assert.ErrorContains(t, err, "missing referenced-data-file
property")
Review Comment:
We cover the absent key but not the present-but-empty value. The guard is
`!ok || referencedDataFile == ""`, and nothing here would fail if someone
trimmed it to `!ok`. Worth a sibling subtest with `props:
{"referenced-data-file": "", "cardinality": "5"}` so that half of the condition
is pinned too.
##########
table/dv/deletion_vector_test.go:
##########
@@ -425,9 +450,55 @@ func TestReadDVInvalidPuffin(t *testing.T) {
assert.ErrorContains(t, err, "create puffin reader")
}
+// Why: offset, size, and cardinality cannot prove that the selected Puffin
blob
+// is a deletion vector for the manifest's referenced data file.
+// Condition: the matched blob has conflicting, missing, or non-DV identity
metadata.
+// Assertion: ReadDV rejects each case before decoding the blob payload.
+func TestReadDVValidatesBlobMetadata(t *testing.T) {
+ dvBlobBytes := readDVTestData(t,
"small-alternating-values-position-index.bin")
+
+ t.Run("mismatched referenced data file", func(t *testing.T) {
Review Comment:
This doesn't actually exercise the redirect it's guarding against.
Every fixture here is a single-blob Puffin file, so this subtest proves we
compare the blob's `referenced-data-file` against the manifest's. It never
proves that `findBlobMetadataByRange` picked the right blob to compare in the
first place.
I'd add a subtest that writes two DV blobs (data-001 and data-002) into one
Puffin file, then calls `ReadDV` with the manifest's `referenced_data_file` set
to data-001 but `(offset, size)` pointing at the data-002 blob. That's the
actual attack. It's also the only case where a future refactor of
`findBlobMetadataByRange` that returns the first blob regardless of offset
would go undetected today.
--
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]
