JingsongLi commented on code in PR #785:
URL: https://github.com/apache/paimon-rust/pull/785#discussion_r3930335975
##########
crates/paimon/src/deletion_vector/core.rs:
##########
@@ -139,59 +156,117 @@ impl DeletionVector {
let mut buf = bytes;
- // Read bitmapLength (total size including magic)
- let bitmap_length = buf.get_i32() as usize;
+ // Read bitmapLength (magic + bitmap data). Both formats store it
+ // big-endian. Reject a negative value here so the size arithmetic
below
+ // cannot wrap: `as usize` would turn -1 into u64::MAX and make the
+ // "data incomplete" guard compute a tiny requirement and pass.
+ let bitmap_length =
+ u64::try_from(buf.get_i32()).map_err(|_| crate::Error::DataInvalid
{
+ message: "Deletion vector bitmap length is
negative".to_string(),
+ source: None,
+ })?;
- // Read magic number
+ // Read magic number. v1 is big-endian, v2 little-endian.
let magic_number = buf.get_i32() as u32;
- if magic_number != MAGIC_NUMBER {
+ let is_bitmap64 = if magic_number == MAGIC_NUMBER {
+ false
+ } else if magic_number.swap_bytes() == MAGIC_NUMBER_64 {
+ true
+ } else {
return Err(crate::Error::DataInvalid {
message: format!(
- "Invalid magic number: expected {MAGIC_NUMBER}, got
{magic_number}"
+ "Invalid magic number: {magic_number}, \
+ v1 dv magic number: {MAGIC_NUMBER}, v2 magic number:
{MAGIC_NUMBER_64}"
),
source: None,
});
- }
+ };
- // Verify length if provided
+ // Verify length if provided, using each format's own convention.
if let Some(expected) = expected_length {
- if bitmap_length as u64 != expected {
+ let expected_bitmap_length = if is_bitmap64 {
+ expected
+ .checked_sub(LENGTH_SIZE_BYTES + CRC_SIZE_BYTES)
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: format!("Deletion vector length {expected} is
too small"),
+ source: None,
+ })?
+ } else {
+ expected
+ };
+ if bitmap_length != expected_bitmap_length {
return Err(crate::Error::DataInvalid {
message: format!(
- "Size not match, actual size: {bitmap_length},
expected size: {expected}"
+ "Size not match, actual size: {bitmap_length},
expected size: {expected_bitmap_length}"
),
source: None,
});
}
}
- // Read bitmap data (bitmapLength - 4 bytes, since magic is already
included in bitmapLength)
- let bitmap_data_size = bitmap_length - MAGIC_NUMBER_SIZE_BYTES;
- // 4(bitmap_length) + 4(magic_number) + bitmap_data_size + 4(crc)
- if bytes.len() < 8 + bitmap_data_size + 4 {
+ // Bitmap data follows the magic, which bitmapLength counts.
+ let bitmap_data_size = bitmap_length
+ .checked_sub(MAGIC_NUMBER_SIZE_BYTES)
+ .and_then(|size| usize::try_from(size).ok())
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: format!("Deletion vector bitmap length
{bitmap_length} is too small"),
+ source: None,
+ })?;
+ // The CRC is not verified, so it need not be present: a v2 entry that
+ // ends the index file cannot be over-read (see
`DeletionVectorFactory::read`).
+ let needed = 8 + bitmap_data_size;
+ if bytes.len() < needed {
return Err(crate::Error::DataInvalid {
message: format!(
- "Deletion vector data incomplete: need {} bytes, got {}",
- 8 + bitmap_data_size + 4,
+ "Deletion vector data incomplete: need {needed} bytes, got
{}",
bytes.len()
),
source: None,
});
}
- let bitmap_data = &bytes[8..8 + bitmap_data_size];
-
- // Skip CRC (4 bytes) - Java code does: dis.skipBytes(4)
- // We don't need to verify it here as it's skipped
+ let bitmap_data = &bytes[8..needed];
+ if is_bitmap64 {
+ Self::from_bitmap64_bytes(bitmap_data)
+ } else {
+ let bitmap =
RoaringBitmap::deserialize_from(bitmap_data).map_err(|e| {
+ crate::Error::DataInvalid {
+ message: format!("Failed to deserialize RoaringBitmap:
{e}"),
+ source: Some(Box::new(e)),
+ }
+ })?;
+ Ok(Self::from_bitmap(bitmap))
+ }
+ }
- // Deserialize RoaringBitmap
- let bitmap = RoaringBitmap::deserialize_from(bitmap_data).map_err(|e| {
+ /// Decode a `Bitmap64DeletionVector` payload into the roaring32
representation
+ /// this type stores.
+ ///
+ /// Row positions are offsets inside a single data file, and every
existing API
+ /// here is already roaring32-bound -- [`Self::is_deleted`] documents that
+ /// positions above `u32::MAX` cannot be present, and `to_bitmap` hands a
+ /// `RoaringBitmap` to the writer. A position that does not fit is
therefore
+ /// rejected rather than truncated, so a 64-bit vector can never silently
lose
+ /// deletes; a data file with more than `u32::MAX` rows would be needed to
+ /// reach it.
+ fn from_bitmap64_bytes(bitmap_data: &[u8]) -> crate::Result<Self> {
+ let treemap =
roaring::RoaringTreemap::deserialize_from(bitmap_data).map_err(|e| {
Review Comment:
[P2] Reject duplicate or out-of-order bitmap64 bucket keys before they can
drop deletes
`roaring::RoaringTreemap::deserialize_from` validates each inner roaring32
bitmap, but its outer decoder simply inserts each `u32` key into a `BTreeMap`;
it does not enforce Java `OptimizedRoaringBitmap64.deserialize`'s requirement
that keys are strictly increasing. A duplicate key therefore overwrites the
earlier bucket silently. If an index is corrupted so two buckets carry key 0,
deleted positions from the first bucket reappear as live rows instead of the
vector being rejected.
I verified this with a two-bucket payload containing key 0 / bitmap {1}
followed by key 0 / bitmap {2}: this method succeeds and returns only {2}; the
equivalent Java reader rejects the second key. Please decode/validate the outer
count and keys explicitly (including nonnegative/range/order checks) before
assembling the treemap, and add duplicate plus descending-key cases.
--
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]
