neilconway commented on code in PR #26453:
URL: https://github.com/apache/datafusion/pull/26453#discussion_r4341745908
##########
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:
x86, branch vs. branchless, with the NULL-slot-fill logic already removed:
```
case branch branchless
change
array_sort/int32/5 265.820 265.530
-0.1% (control)
array_sort/int32/1000 70780.000 71415.000
+0.9% (control)
array_sort/int32_with_nulls 2177.800 2172.300
-0.3% (control)
array_sort/int32_null_elements/5 406.480 384.300
-5.5%
array_sort/int32_null_elements_desc/5 416.660 396.840
-4.8%
array_sort/int32_null_elements_nulls_last/5 387.190 368.940
-4.7%
array_sort/int32_null_elements/20 1291.500 1157.300
-10.4%
array_sort/int32_null_elements_desc/20 1342.800 1209.000
-10.0%
array_sort/int32_null_elements_nulls_last/20 1270.300 1131.400
-10.9%
array_sort/int32_null_elements/100 6078.400 5500.600
-9.5%
array_sort/int32_null_elements_desc/100 6222.500 5624.600
-9.6%
array_sort/int32_null_elements_nulls_last/100 6049.500 5467.700
-9.6%
array_sort/int32_null_elements/1000 73921.000 68853.000
-6.9%
array_sort/int32_null_elements_desc/1000 74745.000 69838.000
-6.6%
array_sort/int32_null_elements_nulls_last/1000 73720.000 68953.000
-6.5%
```
So branchless seems like a clear win. Thanks for the suggestion!
--
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]
