Weijun-H commented on code in PR #6968:
URL: https://github.com/apache/datafusion-comet/pull/6968#discussion_r4277749499
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -1172,7 +1172,39 @@ object CometArrayForAll extends
CometCodegenDispatch[ArrayForAll]
object CometArrayAggregate extends CometCodegenDispatch[ArrayAggregate]
-object CometArraySort extends CometCodegenDispatch[ArraySort]
+object CometArraySort extends CometCodegenDispatch[ArraySort] with
CometTypeShim {
Review Comment:
Done in 11722657f. `CometArraySort` now extends `CometExpressionSerde` like
`CometArrayFilter`, so `array_sort` is listed as Native, with a note on custom
comparators and non-default collations.
##########
spark/src/test/resources/sql-tests/operators/sort_nested_null_order.sql:
##########
@@ -17,13 +17,8 @@
-- Spark orders a null element of an array key, or a null field of a struct
key, below every
-- other value, whatever the key's NULLS FIRST or NULLS LAST, which places
only a null key itself.
--- Comet's native sort places a nested null by the key's null order instead,
so under ASC NULLS
--- LAST and DESC NULLS FIRST a key whose type can hold one falls back to
Spark, in a Sort, a TopK
--- or a window order key
(https://github.com/apache/datafusion-comet/issues/6476). The default
--- null orders, ASC NULLS FIRST and DESC NULLS LAST, put it where Spark does
and stay native.
---
--- The test harness admits incompatible sort orders by default, so turn that
off to check the
--- shipped policy.
+-- For nondefault root NULL placement, Comet wraps the key so only root NULLs
use that placement;
+-- nested NULLs keep Spark's direction-dependent order in Sort, TopK and
window keys.
-- Config: spark.comet.expression.SortOrder.allowIncompatible=false
Review Comment:
Right, nothing reads it anymore. Removed every remaining setting, including
both SQL `Config` lines and the `CometTestBase` override.
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -195,6 +195,44 @@ class CometExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
}
}
+ test("nested sort keys keep root and interior null ordering separate in
native") {
Review Comment:
Removed the duplicate test and moved the `array_sort` coverage into SQL
fixtures, adding the requested element types, empty and all-null arrays, and a
`containsNull = false` input. The collation case is in
`array_sort_collation.sql`.
##########
native/spark-expr/src/array_funcs/sort_array.rs:
##########
@@ -109,6 +109,62 @@ fn constant(value: &ColumnarValue) -> Option<bool> {
}
}
+/// Spark's default `array_sort` comparator. It puts null elements last, but
compares nulls
+/// inside arrays and structs below non-null values.
+#[derive(Debug, Hash, Eq, PartialEq)]
+pub struct SparkArraySort {
+ signature: Signature,
+}
+
+impl Default for SparkArraySort {
+ fn default() -> Self {
+ Self {
+ signature: Signature::any(1, Volatility::Immutable),
+ }
+ }
+}
+
+impl ScalarUDFImpl for SparkArraySort {
+ fn name(&self) -> &str {
+ "spark_array_sort"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ Ok(arg_types[0].clone())
+ }
+
+ fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
+ let [array] = take_function_args(self.name(), &args.args)?;
+ let is_scalar = matches!(array, ColumnarValue::Scalar(_));
+ let rows = if is_scalar { 1 } else { args.number_rows };
+ let array = array.to_array(rows)?;
+ let Some(list) = array.as_list_opt::<i32>() else {
+ return exec_err!("spark_array_sort takes a list, got {}",
array.data_type());
+ };
+ let values = list.values();
+ let compare = spark_comparator(values.as_ref(), values.as_ref())?;
+ let result = sort_by(list, true, |a, b| {
+ match (values.is_null(a), values.is_null(b)) {
Review Comment:
Switched to `spark_comparator_ignoring_nulls` and read the null buffer once,
so arrays without null elements go straight to `sort_by`. Not benchmarked.
--
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]
