comphead commented on code in PR #6968:
URL: https://github.com/apache/datafusion-comet/pull/6968#discussion_r4274439443
##########
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:
`nested sort keys keep root and interior null ordering separate in native`
repeats `sort_nested_null_order.sql`, which already runs `ORDER BY ... ASC
NULLS LAST`, a TopK and a `DENSE_RANK` window over nested null keys, and
`query` mode already asserts the plan is native. Could it go?
The `default array_sort` test below it (line 221) fits an SQL file with `--
Config: spark.comet.exec.scalaUDF.codegen.enabled=false`, next to
`array_sort_comparator.sql`. It only covers `ARRAY<ARRAY<INT>>` today, so a
fixture could add string, binary, decimal, date and timestamp elements, struct
elements with null fields, empty and all-null arrays, and a `containsNull =
false` input such as `array_sort(array(coalesce(i, 0), 1))`. I could not find a
test that reaches the `noNulls` branch of `usesDefaultComparator`. The
collation test in `CometArrayExpressionSuite` could move the same way with `--
MinSparkVersion: 4.0` and `expect_fallback`.
##########
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:
The closure already orders null elements itself, but `spark_comparator`
tests the same top-level nulls again inside every comparison, and
`values.is_null(a)` and `values.is_null(b)` are two further null-buffer lookups
per call. `spark_comparator_ignoring_nulls` exists for callers that apply
top-level nulls themselves and `nested_comparison.rs` already uses it. Would it
work here? Taking `values.nulls().filter(|n| n.null_count() > 0)` once would
also let an array without null elements pass `compare` straight to `sort_by`.
The speedup is an expectation and I did not benchmark it.
##########
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:
`GenerateDocs` labels every plain `CometCodegenDispatch` as `Codegen
dispatch`, so `docs/source/user-guide/latest/expressions.md` keeps `|
array_sort | ✅ | Codegen dispatch | |` although the default comparator now
runs natively. `CometArrayFilter` has the same shape, a native fast path for
one lambda form and the codegen dispatcher for the rest. It extends
`CometExpressionSerde` and its row reads `Native` with a note about the general
lambda going through the dispatcher. Could `CometArraySort` follow that pattern
and the `array_sort` row get a matching note for 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:
After this change `CometSortOrder.getSupportLevel` returns only
`Unsupported` or `Compatible`. On main the only `Incompatible` result was the
nested null order case, so I could not find any remaining consumer of
`spark.comet.expression.SortOrder.allowIncompatible`. If that is right, this
`Config` line, the same line and its comment in
`nested_float_order_keys_strict.sql`, and the
`getExprAllowIncompatConfigKey("SortOrder") -> "false"` settings the PR adds in
`CometExpressionSuite` and `CometWindowExecSuite` have no effect. The override
in `CometTestBase` and its comment about mixed zero and negative zero are stale
for the same reason.
--
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]
