dwsmith1983 commented on code in PR #6945:
URL: https://github.com/apache/datafusion-comet/pull/6945#discussion_r4265806115
##########
native/core/src/execution/delta_dv.rs:
##########
@@ -1120,12 +1120,30 @@ mod tests {
#[test]
fn native_format_decodes() {
- let values = [0u64, 2, 3, 100, (1u64 << 32) + 7];
+ let values = [0u64, 2, 3, 100, (2u64 << 32) + 7];
let decoded = deserialize_dv_bitmap(&native_bytes(&values)).unwrap();
let expected: RoaringTreemap = values.into_iter().collect();
assert_eq!(decoded, expected);
}
+ #[test]
+ fn native_decode_preserves_dense_bitmap_compression() {
+ let mut bitmap = RoaringBitmap::new();
+ bitmap.insert_range(0..1_000_000);
+ let compressed_size = bitmap.serialized_size();
+ let mut bitmap_bytes = Vec::new();
+ bitmap.serialize_into(&mut bitmap_bytes).unwrap();
+
+ let mut data = NATIVE_MAGIC.to_le_bytes().to_vec();
+ data.extend(1i32.to_le_bytes());
+ data.extend((bitmap_bytes.len() as i32).to_le_bytes());
+ data.extend(bitmap_bytes);
+
+ let decoded = deserialize_dv_bitmap(&data).unwrap();
+ let (_, decoded_bitmap) = decoded.bitmaps().next().unwrap();
+ assert_eq!(decoded_bitmap.serialized_size(), compressed_size);
Review Comment:
This pins the container type, which is the point of the PR. Could it also
push the decoded treemap through `build_access_plan`? Something like
`full_file_access_plan(&[600_000, 600_000], &decoded)` with group 0 expected as
`Skip` and group 1 as skip 400_000 then select 200_000, plus
`assert_eq!(decoded.len(), 1_000_000)`. Nothing in this file currently drives a
run container through the plan builder, and after this change dense native DVs
will.
##########
native/core/src/execution/delta_dv.rs:
##########
@@ -1120,12 +1120,30 @@ mod tests {
#[test]
fn native_format_decodes() {
- let values = [0u64, 2, 3, 100, (1u64 << 32) + 7];
+ let values = [0u64, 2, 3, 100, (2u64 << 32) + 7];
Review Comment:
Worth making the intent visible here, since this value is what guards the
empty-partition skip. Either a short comment ("key 1 is an empty partition, key
2 must keep its index") or an assertion on `decoded.bitmaps()` yielding keys
`[0, 2]`.
--
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]
