viirya commented on code in PR #6824:
URL: https://github.com/apache/datafusion-comet/pull/6824#discussion_r4234209551
##########
native/shuffle/src/spark_unsafe/list.rs:
##########
@@ -39,55 +44,90 @@ use datafusion_comet_jni_bridge::errors::CometError;
/// time with the copy, and arrays of 50 about a fifth.
const MIN_BULK_APPEND_ELEMENTS: usize = 8;
+/// Element count from which an aligned nullable array that holds a null is
appended with one copy
+/// of its values and a validity buffer built from its null bitset, instead of
element by element.
+/// Building the two buffers takes four allocations, which cost as much as
appending 40 to 55
+/// elements one by one, so shorter arrays stay on the loop: with the copy,
arrays of 16 elements
+/// took 2.4 times as long and arrays of 32 elements 1.3 times. Arrays of 64
elements take 65% of
+/// the time with the copy for 4-byte elements and 85% for 8-byte ones, and
arrays of 1024 a ninth
+/// and a third.
+const MIN_BULK_NULLABLE_APPEND_ELEMENTS: usize = 64;
Review Comment:
Thanks for adding `list_with_one_null`. I ran it as its doc comment
describes, against a baseline with the constant at `usize::MAX`, on an M-series
Mac. This goes through `AccountingAllocator`, since the bench links the `comet`
crate. At 64 elements the bulk path took 55% less time for `i32` and 45% less
for `i64`, and at 128 elements 71% and 65% less. That is a wider margin than
the out-of-tree harness showed, so the accounting wrapper doesn't eat into it.
The 32-element cases run the same loop on both sides, and their differences
were within noise. I'm fine with the cutoff as it is.
##########
native/core/benches/array_element_append.rs:
##########
@@ -137,6 +147,27 @@ fn create_spark_unsafe_array_f64(num_elements: usize,
with_nulls: bool) -> Vec<u
buffer
}
+/// Create a SparkUnsafeArray of `num_elements` elements of `width` bytes
whose element `null_idx`
+/// is null, on an 8-byte boundary as inside an UnsafeRow.
+fn create_spark_unsafe_array_with_one_null(
+ num_elements: usize,
+ width: usize,
+ null_idx: usize,
+) -> Vec<u64> {
+ let header_size = 8 + num_elements.div_ceil(64) * 8;
+ let mut buffer = vec![0u8; header_size + (num_elements *
width).div_ceil(8) * 8];
+ buffer[0..8].copy_from_slice(&(num_elements as i64).to_le_bytes());
+ buffer[8 + null_idx / 8] |= 1 << (null_idx % 8);
+ for i in 0..num_elements {
+ let offset = header_size + i * width;
+ buffer[offset..offset + width].copy_from_slice(&(i as
i64).to_le_bytes()[..width]);
+ }
+ buffer
+ .chunks_exact(8)
Review Comment:
This is what fails `ubuntu-latest/rust-test`. Clippy flags `chunks_exact(8)`
with `clippy::chunks_exact_to_as_chunks` under `-D warnings`, so the job stops
before the Rust tests run. Its suggestion `as_chunks::<8>().0.iter()` should
work here, since the buffer length is already a multiple of 8.
--
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]
