feat(delete-vector): parse deletion-vector-v1 puffin blob - read support#2414
feat(delete-vector): parse deletion-vector-v1 puffin blob - read support#2414Shekharrajak wants to merge 1 commit into
Conversation
7ce78ba to
f625093
Compare
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn deserialize_blob(blob_bytes: &[u8], expected_cardinality: Option<u64>) -> Result<Self> { |
There was a problem hiding this comment.
Read path deserialize_blob parses a binary blob (length + magic + roaring bitmap + CRC32) into a DeleteVector.
|
|
||
| #[allow(dead_code)] | ||
| pub fn deserialize_blob(blob_bytes: &[u8], expected_cardinality: Option<u64>) -> Result<Self> { | ||
| if blob_bytes.len() < DV_MIN_BLOB_SIZE { |
There was a problem hiding this comment.
Minimum size check
| } | ||
|
|
||
| // 2GB cap matches Java's signed-i32 length field. | ||
| if blob_bytes.len() > i32::MAX as usize { |
There was a problem hiding this comment.
2GB cap - Java's Integer.MAX_VALUE is 2,147,483,647 — exactly i32::MAX - similar to java implementation
|
Hi @Shekharrajak could you please update the PR description to make review easier? |
| bimap = { workspace = true } | ||
| bytes = { workspace = true } | ||
| chrono = { workspace = true } | ||
| crc32fast = { workspace = true } |
There was a problem hiding this comment.
crc32fast — fast CRC32 over the blob payload
f625093 to
5c928e2
Compare
|
Hi @mbutrovich , Added short PR decription. It is pretty much same logic as Iceberg java implementation. I can add more unit tests going forward when we start write support . |
|
@c-thiel can you please review ? |
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @Shekharrajak. I read this as the decode half of deletion-vector read support (Task 1 of #2792, under the v3 epic #2411).
The approach is sound. Leaning on roaring's RoaringTreemap::serialize_into / deserialize_from is the right call. In roaring 0.11.x that path is the official portable 64-bit format (8-byte little-endian bitmap count, ascending 4-byte little-endian key, standard 32-bit bitmap), so it matches Iceberg-Java and iceberg-go without hand-rolling the framing. The length, magic, and CRC handling line up with BitmapPositionDeleteIndex, hoisting the sizes and magic into named constants is good, and the error-path tests are thorough.
The most important change is the golden fixtures. They are generated by the crate itself, so they do not validate against Java, and they never exercise run-optimized bitmaps, which is the case most likely to break a cross-engine read. Detail is inline on the regenerate test.
The rest, all inline: verify the CRC before parsing the payload, add a doc comment on deserialize_blob that pins down the byte layout and the expected_cardinality cross-check, drop the dead_code allows once the reader wiring lands, and settle the method name now so it does not get renamed across #2678 and #2681 when they land together.
| /// Regenerate the committed golden fixtures (see testdata README). | ||
| #[test] | ||
| #[ignore] | ||
| fn dv_blob_regenerate_golden_fixtures() { |
There was a problem hiding this comment.
This regenerates the fixtures with serialize_blob_for_test, which encodes through roaring's serialize_into. So the committed fixtures are produced by this crate, not by Iceberg-Java, and the dv_blob_golden_* tests validate the codec against its own encoder rather than against a reference. The PR description says the fixtures come from the Java reference writer, but this test overwrites them with roaring-encoded bytes.
It also leaves a real gap. Java runs runLengthEncode (runOptimize) on each 32-bit bitmap before serializing, so production DVs use RUN containers. roaring's serialize_into does not run-optimize, so none of these fixtures decode a RUN container, which is the case most likely to break cross-engine reads.
Commit at least one fixture produced by Iceberg-Java, ideally a run-heavy one like the dense range, document the exact command that generated it in the README, and either drop this regenerate test or gate it so it cannot silently replace the reference bytes. #2678 already committed Java-verified DV fixtures under testdata/puffin that can be reused here.
|
|
||
| let roaring_end = blob_bytes.len() - DV_CRC_SIZE; | ||
| let mut cursor = Cursor::new(&blob_bytes[magic_end..roaring_end]); | ||
| let inner = RoaringTreemap::deserialize_from(&mut cursor).map_err(|e| { |
There was a problem hiding this comment.
Check the CRC before deserializing the roaring payload. Right now the payload is parsed here and the CRC is verified at line 141, so a corrupted blob fails with an opaque roaring parse error instead of a clear CRC mismatch, and dv_blob_rejects_bad_crc having to accept either error is the symptom. The CRC is a single cheap pass over blob_bytes[4..len-4], so verifying it first gives a deterministic error and keeps corrupt bytes out of the parser.
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn deserialize_blob(blob_bytes: &[u8], expected_cardinality: Option<u64>) -> Result<Self> { |
There was a problem hiding this comment.
Add a doc comment on deserialize_blob, matching the style of insert_positions above it. State the layout it expects ([length][magic][roaring][crc32]), document that expected_cardinality is an optional cross-check against the manifest record_count and is skipped when None, and list the error conditions, with a link to the Puffin spec deletion-vector-v1 section. That keeps the byte-format contract next to the code and settles where the cardinality check lives.
|
|
||
| use crate::{Error, ErrorKind, Result}; | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
These constants and deserialize_blob are all #[allow(dead_code)] because nothing calls them in this PR. Add a reference to the read tracking issue #2792 next to them, and remove the allows when the loader wiring lands, so they do not become permanent.
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn deserialize_blob(blob_bytes: &[u8], expected_cardinality: Option<u64>) -> Result<Self> { |
There was a problem hiding this comment.
|
Since the fixture gap came up: we have real On naming: no attachment to |
|
Closing in favor of #2866. Thank you for the initial PR, @Shekharrajak! |
Which issue does this PR close?
Ref #2411
What changes are included in this PR?
Implements DeleteVector::deserialize_blob to parse Iceberg V3 deletion-vector-v1 Puffin blobs into a RoaringTreemap of deleted row positions.
• Iceberg V3 spec — Deletion Vectors: https://iceberg.apache.org/spec/#deletion-vectors
• Iceberg V3 spec — Puffin file format: https://iceberg.apache.org/puffin-spec/
• Java reference implementation: org.apache.iceberg.deletes.PositionDeleteIndex and BaseDVFileWriter
Are these changes tested?
Five binary fixtures generated by the Java reference writer and validation using unit test.