Jefffrey commented on code in PR #24823:
URL: https://github.com/apache/datafusion/pull/24823#discussion_r3900315797
##########
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:
are there performance implications here? im not sure our benchmarks cover a
descending case
##########
datafusion/functions-nested/src/array_has.rs:
##########
Review Comment:
we could even get rid of this thin wrapper function since its only called at
one place; all it does is destructure/cast input arrays
--
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]
