jayzhan211 commented on code in PR #24823:
URL: https://github.com/apache/datafusion/pull/24823#discussion_r3904821073
##########
datafusion/functions-nested/src/sort.rs:
##########
@@ -238,99 +238,104 @@ fn sort_primitive_list<T: ArrowPrimitiveType,
OffsetSize: OffsetSizeTrait>(
where
T::Native: ArrowNativeTypeOp,
{
- if prim_values.null_count() > 0 {
- sort_list_with_nulls(prim_values, list_array, field, sort_options)
- } else {
- sort_list_no_nulls(prim_values, list_array, field, sort_options)
+ let descending = sort_options.is_some_and(|o| o.descending);
+ let nulls_first = sort_options.is_none_or(|o| o.nulls_first);
+ let list_nulls = list_array.nulls();
+ let offsets = list_array.offsets();
+
+ let (values, validity) = match prim_values.nulls() {
+ Some(element_nulls) if element_nulls.null_count() > 0 => {
+ let (values, validity) = sort_rows_with_nulls(
+ prim_values.values(),
+ element_nulls,
+ offsets,
+ list_nulls,
+ descending,
+ nulls_first,
+ );
+ (values, Some(validity))
+ }
+ _ => (
+ sort_rows_no_nulls(prim_values.values(), offsets, list_nulls,
descending),
+ None,
+ ),
+ };
+
+ let sorted_values = Arc::new(
+ PrimitiveArray::<T>::new(values.into(), validity)
+ .with_data_type(prim_values.data_type().clone()),
+ );
+
+ Ok(Arc::new(GenericListArray::<OffsetSize>::try_new(
+ field,
+ rebase_offsets(offsets),
+ sorted_values,
+ list_nulls.cloned(),
+ )?))
+}
+
+/// Sorts one row's values in place.
+///
+/// [`ArrowNativeTypeOp::compare`] is a total order, so a descending sort is
the
+/// reverse of the ascending one. Sorting one way and reversing keeps a single
+/// instantiation of the standard library's sort per native type, instead of
one
+/// per comparator closure.
+#[inline]
+fn sort_row<N: ArrowNativeTypeOp>(row: &mut [N], descending: bool) {
+ row.sort_unstable_by(|a, b| a.compare(*b));
+ if descending {
+ row.reverse();
Review Comment:
I didn't add a descending bench case, but I measured it locally (8192 rows,
Int32, this branch vs `main`, negative = `main` faster):
| elements/row | 5 | 20 | 100 | 1000 |
|---|---|---|---|---|
| `desc` | −1.6% | −1.1% | −0.04% | −0.2% |
| `desc`, null elements | +5.6% | +7.7% | −4.1% | −1.1% |
Re-running the identical build against itself swings ±3%, so this is all
noise-level — the extra `reverse()` is an O(n) pass on an O(n log n) sort.
Keeping it means one `sort_unstable_by` instantiation per native type instead
of one per comparator, which is the code size this PR is cutting.
--
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]
