zeroshade commented on code in PR #1615:
URL: https://github.com/apache/iceberg-go/pull/1615#discussion_r3732159440
##########
table/dv/deletion_vector_test.go:
##########
@@ -425,9 +468,95 @@ 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) {
Review Comment:
Non-blocking: Consider extending this coverage with a scan-layer propagation
case proving an identity mismatch escapes `readAllDeletionVectors`, an explicit
exact-match case (`s3a://...` versus `s3://...`), and a
compression-codec-bearing blob. Lower-priority additions would use
`BlobTypeDataSketchesTheta` for the wrong-type collision and add a Go-written
positive multi-blob case.
##########
table/dv/deletion_vector.go:
##########
@@ -146,6 +149,14 @@ func SerializeDV(bitmap *RoaringPositionBitmap) ([]byte,
error) {
// stale manifest record_count against a freshly written blob) is a writer bug
// and fails fast. The bitmap is then validated against the manifest count.
//
+// ReadDV also requires the selected blob to be a deletion-vector blob whose
Review Comment:
Non-blocking: Consider tightening this documentation by naming Java's
`DVUtil.readDV`, which makes the interoperability comparison directly
checkable, and collapsing the repeated missing-cardinality rationale now
present at lines 157-163.
##########
table/dv/deletion_vector.go:
##########
@@ -158,6 +169,10 @@ func ReadDV(fs iceio.IO, dvFile iceberg.DataFile)
(*RoaringPositionBitmap, error
if dvFile.ContentOffset() == nil || dvFile.ContentSizeInBytes() == nil {
return nil, fmt.Errorf("DV file %s missing
ContentOffset/ContentSizeInBytes", dvFile.FilePath())
}
+ manifestReferencedDataFile := dvFile.ReferencedDataFile()
+ if manifestReferencedDataFile == nil || *manifestReferencedDataFile ==
"" {
+ return nil, fmt.Errorf("DV file %s missing ReferencedDataFile",
dvFile.FilePath())
Review Comment:
Non-blocking: Consider spelling this field `referenced_data_file` to match
the specification and the mismatch diagnostic below. Since the new identity
failures otherwise expose only strings, a package sentinel wrapped with `%w`
would also let operator tooling distinguish metadata corruption with
`errors.Is`.
##########
table/dv/deletion_vector.go:
##########
@@ -176,9 +191,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 {
Review Comment:
Non-blocking: Consider rejecting a non-empty `blob.CompressionCodec` here.
`deletion-vector-v1` omits the compression codec, and this path reads the blob
bytes raw; a blob declaring zstd currently falls through to a confusing
`invalid deletion vector magic` error instead of reporting the metadata
violation.
--
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]
