CTTY commented on code in PR #2866:
URL: https://github.com/apache/iceberg-rust/pull/2866#discussion_r3771411568
##########
crates/iceberg/src/delete_vector.rs:
##########
@@ -68,6 +76,88 @@ impl DeleteVector {
pub fn len(&self) -> u64 {
self.inner.len()
}
+
+ /// Parses a `deletion-vector-v1` Puffin blob into a `DeleteVector`.
+ ///
+ /// The layout, defined by the Iceberg Puffin spec and matching
Iceberg-Java's
+ /// `BitmapPositionDeleteIndex`, is:
+ ///
+ /// ```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 a roaring bitmap in
the portable
+ /// 64-bit format read by [`RoaringTreemap::deserialize_from`].
+ ///
+ /// Cardinality is not checked here. The caller validates the decoded
length against the
+ /// delete file's `record_count`, where the manifest metadata is available.
+ ///
+ /// # Errors
+ ///
+ /// Returns [`ErrorKind::DataInvalid`] if the blob is shorter than the
minimum, the length
+ /// prefix or CRC does not match, the magic is wrong, or the roaring
payload fails to decode.
+ // Consumed by the scan delete loader once the deletion-vector read path
is wired up.
+ #[allow(dead_code)]
+ pub fn deserialize(blob: &[u8]) -> Result<Self> {
+ if blob.len() < DV_MIN_BLOB_BYTES {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion-vector-v1 blob is {} bytes, shorter than the
{DV_MIN_BLOB_BYTES}-byte minimum",
+ blob.len()
+ ),
+ ));
+ }
+
+ // The magic and vector, i.e. the bytes covered by both the length
prefix and the CRC.
+ let body = &blob[DV_LENGTH_PREFIX_BYTES..blob.len() - DV_CRC_BYTES];
+
+ let declared_len =
+
u32::from_be_bytes(blob[..DV_LENGTH_PREFIX_BYTES].try_into().unwrap()) as usize;
+ if declared_len != body.len() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion-vector-v1 length prefix is {declared_len},
expected {}",
+ body.len()
+ ),
+ ));
+ }
+
+ // Verify the CRC before interpreting any bytes so a corrupt blob
yields a single clear
+ // error rather than an opaque roaring decode failure.
+ let stored_crc = u32::from_be_bytes(blob[blob.len() -
DV_CRC_BYTES..].try_into().unwrap());
+ let computed_crc = crc32fast::hash(body);
+ if computed_crc != stored_crc {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion-vector-v1 CRC mismatch: computed
{computed_crc:#010x}, stored {stored_crc:#010x}"
+ ),
+ ));
+ }
+
+ let (magic, vector) = body.split_at(DV_MAGIC_BYTES);
+ if magic != DV_MAGIC {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion-vector-v1 magic mismatch: {magic:02x?}, expected
{DV_MAGIC:02x?}"
+ ),
+ ));
+ }
+
+ let inner = RoaringTreemap::deserialize_from(vector).map_err(|e| {
Review Comment:
Just read java's implementation and noticed that we never verify the range
and the order of the key here.
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java#L302
--
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]
