neilconway commented on code in PR #26453:
URL: https://github.com/apache/datafusion/pull/26453#discussion_r4341431347
##########
datafusion/functions-nested/src/sort.rs:
##########
@@ -338,28 +339,29 @@ fn sort_rows_with_nulls<N: ArrowNativeTypeOp, O:
OffsetSizeTrait>(
continue;
}
- let null_count = src_nulls.slice(start, row_len).null_count();
- let valid_count = row_len - null_count;
-
- // Compact valid values directly into the target region of the output
- // buffer: after nulls (if nulls_first) or at the start (if
nulls_last).
- let valid_offset = if nulls_first { null_count } else { 0 };
- let mut write_pos = out_start + valid_offset;
- for (i, &value) in src_values[start..end].iter().enumerate() {
- if src_nulls.is_valid(start + i) {
+ // Compact valid values to the start of the row's output region,
+ // counting them on the way, and sort them there.
+ let validity_bits =
+ BitIterator::new(src_nulls.validity(), src_nulls.offset() + start,
row_len);
+ let mut write_pos = out_start;
+ for (&value, is_valid) in
src_values[start..end].iter().zip(validity_bits) {
+ if is_valid {
out_values[write_pos] = value;
write_pos += 1;
}
}
+ let valid_count = write_pos - out_start;
+ let null_count = row_len - valid_count;
Review Comment:
Thanks for the suggestion! Seems promising; I'm running a benchmark now on
x86.
> The loop can leave the last NULL's value in the slot right after the valid
values. The suggestion resets that slot, so NULL slots stay zeroed the way the
NULLS FIRST branch keeps them.
On reflection, why don't we just avoid making all NULL slots contain
`N::default()` to begin with, and let them hold whatever they would otherwise
hold? It saves a few cycles, and is probably net more readable (less code and
no need to explain what's happening in the special-case).
--
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]
