fix(deletion_vector): read Java bitmap64 deletion vectors - #785
fix(deletion_vector): read Java bitmap64 deletion vectors#785jackylee-ch wants to merge 2 commits into
Conversation
| /// 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| { |
There was a problem hiding this 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.
|
Confirmed. Outer layer is decoded here now, mirroring |
A deletion vector written by Java in the 64-bit format (
Bitmap64DeletionVector)cannot be read:
read_from_bytesaccepts only the v1 magic, so every scan touchingsuch a file fails with
Invalid magic number, naming the v1 magic alone.Java's
DeletionVector.readdispatches on both.Two things differ beyond the value. The v2 magic is written little-endian, and
DeletionFile.length()counts the length prefix and the CRC for v2 but neither forv1. The second also breaks the read range: the factory asked for
length + 8, pastthe end of a v2 entry, so a vector ending its index file was rejected by the storage
layer before parsing.
Fix: detect either magic, apply each format's length convention, and decode the
payload's outer layer here rather than through
RoaringTreemap::deserialize_from—that decoder validates each inner roaring32 bitmap but inserts bucket keys straight
into a map, so a duplicate key silently replaces a bucket and its deletes vanish.
Java rejects that in
OptimizedRoaringBitmap64.readBitmapCount/readKey, and thesame checks are applied: count range, key bounds, strictly ascending. A duplicate
key, a descending key, a bogus count or a truncated payload now error instead of
decoding partially. Java run-length encodes before writing and emits every bucket
key densely, so tests cover both shapes.
Read side only; the writer still emits v1. Positions above
u32::MAXare rejectedrather than truncated — every API here is already roaring32-bound, and reaching one
needs a data file with over 4.29e9 rows. A negative
bitmapLengthread from thefile now errors instead of wrapping the size guard into passing.