feat(delete-vector): [1/N] decode deletion-vector-v1 puffin blobs - #2866
Conversation
Add DeleteVector::deserialize to parse a deletion-vector-v1 blob ([length][magic][roaring][crc32], portable 64-bit roaring) into a DeleteVector. Verifies the length prefix, CRC-32, and magic before decoding, and leaves cardinality validation to the caller that holds the delete file's record_count. Tests cover round-trips (including a run-optimized bitmap that exercises the RUN-container decode path) and the length, magic, and CRC error paths. Refs apache#2792.
hsiang-c
left a comment
There was a problem hiding this comment.
Thanks @mbutrovich, LGTM
CTTY
left a comment
There was a problem hiding this comment.
LGTM! left some minor comments
|
Thanks for the review, @CTTY! I'll take a pass on your feedback tomorrow. I appreciate the time spent! |
|
Thanks again @CTTY! Sorry for the delay. This should be ready for another pass. |
| // earlier bitmap on a duplicate) so a non-conformant blob is rejected instead of decoded | ||
| // into a value that doesn't match what was actually written. | ||
| let mut reader = vector; | ||
| let bitmap_count = reader.try_get_u64_le().map_err(|e| { |
There was a problem hiding this comment.
Roaring bitmap portable format states that the range or bitmap_count actuallys falls in [0, 2^32 - 1], and we should validate that.
I think 2^32 - 1 is the number of possible keys. I'm not really sure why they want to use 4 padding zero bytes here instead of just use 4 bytes to represent bitmap_count. But not validating this will allow unexpected failure
There was a problem hiding this comment.
Also can we move these validations into helper functions to improve readability?
The body of the current function is mainly different validations right now and is a bit hard to follow
There was a problem hiding this comment.
Also can we move these validations into helper functions to improve readability?
The body of the current function is mainly different validations right now and is a bit hard to follow
Done, split into verify_length_prefix, verify_crc, verify_magic, and decode_roaring_directory. deserialize now just threads the blob through them in order.
There was a problem hiding this comment.
Roaring bitmap portable format states that the range or bitmap_count actuallys falls in [0, 2^32 - 1], and we should validate that.
I think 2^32 - 1 is the number of possible keys. I'm not really sure why they want to use 4 padding zero bytes here instead of just use 4 bytes to represent bitmap_count. But not validating this will allow unexpected failure
Good catch, added. The roaring portable format spec restricts the bitmap count to [0, 2^32 - 1] (stored as a u64 with the top 32 bits reserved as padding), so I added a bound check before the per-key loop in decode_roaring_directory.
I didn't add Java's additional key <= Integer.MAX_VALUE - 1 bound on individual keys: that's an artifact of RoaringBitmap using a signed 32-bit int internally in Java, not a requirement in the Puffin spec or the Roaring format spec. Our key is already a plain u32, so it's structurally confined to [0, 2^32-1] with no extra check needed.
Which issue does this PR close?
deletion-vector-v1Puffin blobs) #2792.What changes are included in this PR?
Adds
DeleteVector::deserialize, which decodes adeletion-vector-v1Puffin blob into aDeleteVector(aRoaringTreemapof deleted row positions). This is the first, self-contained piece of deletion-vector read support (epic #2792): it adds no scan, reader, or planning changes and can be reviewed in isolation.The blob layout follows the Iceberg Puffin spec and matches Iceberg-Java's
BitmapPositionDeleteIndex/RoaringPositionBitmap:lengthcounts the magic and vector bytes (not itself or the CRC).crc32fast) covers the magic and vector.vectoris a roaring bitmap in the portable 64-bit format: a directory of 32-bit key / bitmap pairs, ordered by unsigned comparison of the keys (roaring 0.11.x implements this official format). The directory is walked manually rather than viaRoaringTreemap::deserialize_from, which silently accepts duplicate or out-of-order keys and can drop positions; walking it ourselves rejects a non-conformant blob instead of mis-decoding it.Validation order is length prefix, then CRC (before decoding any payload), then magic, then the roaring decode, so a corrupt blob yields a single clear error rather than an opaque roaring failure. Cardinality is intentionally not checked here; the caller validates the decoded length against the delete file's
record_countonce the manifest metadata is available (a later PR in the epic).Also adds
crc32fastas a dependency.This overlaps with #2414, which implemented the same decode. That PR has been idle since review feedback, so this is submitted in its place to keep the epic moving; thanks to @Shekharrajak for the original contribution and for informing this implementation.
Are these changes tested?
Yes, unit tests in
delete_vector.rs:RoaringTreemap::optimize) so the RUN-container decode path is exercised, since Iceberg-Java run-length-encodes deletion vectors before writing.This decode path is also exercised end to end against deletion vectors written by Spark / Iceberg-Java in a draft DataFusion Comet PR (apache/datafusion-comet#4887), which reads real V3 merge-on-read tables through this code and verifies the deleted rows match Spark. That gives cross-implementation confidence that it decodes what Iceberg actually writes, not just what these tests encode.
AI Disclosure
Developed with the help of Claude Code, but I understand and support these changes.