JeonDaehong commented on code in PR #3232:
URL: https://github.com/apache/iceberg-rust/pull/3232#discussion_r4181995275
##########
crates/iceberg/src/delete_vector.rs:
##########
@@ -122,6 +122,52 @@ impl DeleteVector {
Ok(DeleteVector { inner })
}
+
+ /// Serializes this `DeleteVector` into a `deletion-vector-v1` Puffin blob
that
+ /// [`DeleteVector::deserialize`] reads back, conforming to the Iceberg
Puffin
+ /// `deletion-vector-v1` layout:
+ ///
+ /// ```text
+ /// [length: u32 big-endian][magic: D1 D3 39 64][vector][crc: u32
big-endian]
+ /// ```
+ ///
+ /// `length` counts the magic and vector bytes (not itself or the CRC).
The CRC-32 is computed
+ /// over the magic and vector. `vector` is the roaring bitmap in the
portable 64-bit format: a
+ /// directory of 32-bit key / 32-bit roaring bitmap pairs ordered by
unsigned comparison of the
+ /// keys, one bitmap per key. `roaring`'s `RoaringTreemap::serialize_into`
emits exactly that
+ /// directory (a `u64` little-endian count followed by ascending
`u32`-keyed bitmaps), which the
+ /// round-trip tests verify against [`decode_roaring_directory`].
+ ///
+ /// The roaring vector is written as-is, not run-length-encoded, using
sparse-key encoding: only
+ /// the non-empty 32-bit containers are emitted. For the same logical set
of positions these
+ /// bytes therefore need not match Iceberg-Java's
`BitmapPositionDeleteIndex` output, which
+ /// run-optimizes the bitmap and writes dense keys. Both encodings are
`deletion-vector-v1`
+ /// spec-conformant and decode to the same positions, so each side reads
back what the other
+ /// writes. Callers that want size-optimal, Java-like blobs should
`optimize()` the underlying
+ /// roaring bitmap (`RoaringTreemap::optimize`) before serializing.
+ ///
+ /// This produces the raw blob only; wrapping it in a Puffin `Blob` with
the associated
+ /// snapshot and sequence-number properties is handled separately.
+ ///
+ /// # Errors
+ ///
+ /// Returns [`ErrorKind::DataInvalid`] if the framed body (the magic plus
the vector) would
+ /// exceed `u32::MAX` bytes, since the `deletion-vector-v1` length prefix
is a `u32`; reaching
+ /// this requires a deletion vector of roughly 4 GiB, far larger than any
realistic set of row
+ /// positions. Returns [`ErrorKind::Unexpected`] if writing the roaring
treemap into the
+ /// in-memory buffer fails, which does not happen under Rust's allocator
model (an allocation
+ /// failure aborts rather than returning an error); it guards only against
a hypothetical future
+ /// change to `roaring`'s API that would make `serialize_into` fallible
for an in-memory `Vec`.
+ // Nothing in non-test crate code calls this yet (the delete-vector write
path wires it in
+ // later), and `delete_vector` is a private module, so it otherwise reads
as dead code.
+ #[allow(unused)]
+ pub fn serialize(&self) -> Result<Vec<u8>> {
Review Comment:
The spec says the vector supports positive 64-bit positions ("the most
significant bit must be 0"), but insert accepts any u64 and serialize doesn't
check, so a position >= 2^63 would be written as a key >= 2^31. Iceberg Java
rejects that on read (RoaringPositionBitmap.readKey requires key <=
Integer.MAX_VALUE - 1), so the "mutually decodable" note wouldn't hold for that
range.
Real row positions won't get there, so this is mostly defensive, but would
it make sense for serialize to return an error when the highest key has its top
bit set, like the length check below? That would keep us from writing a blob
other implementations can't read.
--
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]
