mbutrovich commented on code in PR #26453:
URL: https://github.com/apache/datafusion/pull/26453#discussion_r4341461791
##########
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:
> 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).
Sounds reasonable to me!
--
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]
