kosiew commented on code in PR #25494:
URL: https://github.com/apache/datafusion/pull/25494#discussion_r4227847253
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1360,21 +1361,56 @@ fn pre_selection_scatter(
let mut last_end = 0;
match right_result {
Some(right_result) => {
+ // Borrow the RHS buffers once. `BooleanArray::slice` clones and
+ // drops an `Arc` for every run, which dominates this loop when the
+ // mask selects many short runs.
+ let right_values = right_result.values();
+ let right_offset = right_values.offset();
+ let right_bytes = right_values.values();
+ let right_nulls = right_result.nulls();
+
+ let mut values = BooleanBufferBuilder::new(result_len);
Review Comment:
Could we move `result_array_builder` initialization into the `None` branch?
The new `Some` branch allocates its own bitmap builder and returns without
using the original one, so this would avoid an unnecessary allocation and free.
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1360,21 +1361,56 @@ fn pre_selection_scatter(
let mut last_end = 0;
match right_result {
Some(right_result) => {
+ // Borrow the RHS buffers once. `BooleanArray::slice` clones and
+ // drops an `Arc` for every run, which dominates this loop when the
+ // mask selects many short runs.
+ let right_values = right_result.values();
+ let right_offset = right_values.offset();
+ let right_bytes = right_values.values();
+ let right_nulls = right_result.nulls();
+
+ let mut values = BooleanBufferBuilder::new(result_len);
+ // Only build a validity bitmap when the RHS has nulls. Otherwise
+ // every run pays a second pass to write an all-ones mask that is
+ // then discarded.
+ let mut validity = right_nulls.map(|_|
BooleanBufferBuilder::new(result_len));
+
SlicesIterator::new(mask).for_each(|(start, end)| {
if start > last_end {
- result_array_builder.append_n(start - last_end,
fill_value);
+ let gap = start - last_end;
+ values.append_n(gap, fill_value);
+ if let Some(v) = validity.as_mut() {
+ v.append_n(gap, true);
+ }
}
- // Copy the selected RHS slice.
+ // Both sides are bit-packed, so copy the run a word at a
+ // time. Iterating the RHS would yield one `Option<bool>` and
+ // set one bit per row.
let len = end - start;
- right_result
- .slice(right_array_pos, len)
- .iter()
- .for_each(|v| result_array_builder.append_option(v));
+ let from = right_offset + right_array_pos;
+ values.append_packed_range(from..from + len, right_bytes);
Review Comment:
Could we add a scatter regression test covering nullable RHS values,
non-byte-aligned offsets, runs crossing byte and 64-bit boundaries, and
leading, intermediate, and trailing gaps with both fill values? Please include
independently offset value and validity buffers, since ordinary slices usually
share the same offset, and compare the output against a simple row-wise
reference.
--
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]
