andygrove opened a new pull request, #6683: URL: https://github.com/apache/datafusion-comet/pull/6683
## Which issue does this PR close? Closes #6476. ## Rationale for this change Spark orders a null element of an array sort key, or a null field of a struct sort key, below every other value, whatever the key's `NULLS FIRST` or `NULLS LAST`, which only places a null key itself. Comet's native sort places a nested null by the key's null order instead. The default null orders, `ASC NULLS FIRST` and `DESC NULLS LAST`, put it where Spark does, but under `ASC NULLS LAST` and `DESC NULLS FIRST` a Sort, a TopK or a window order key returns rows in a different order from Spark, with no fallback and no error. On `main`, `SELECT id FROM t ORDER BY array(i) DESC NULLS FIRST, id` returns `2, 3, 1` where Spark returns `3, 1, 2`, and `RANK() OVER (ORDER BY array(i) DESC NULLS FIRST)` gives the null row rank 1 instead of 3. This is the fallback-first fix. A native fix can follow, for example by sorting on `k IS NULL` ahead of the key with the default null order, which keeps a null key where the user asked for it and puts nested nulls where Spark does. ## What changes are included in this PR? `CometSortOrder.getSupportLevel` reports a key `Incompatible` when its type can hold a null element or field (the existing `canHoldNestedNull`) and its null order is not the default for its direction. Sort, TopK (`TakeOrderedAndProject`), `Window` and `WindowGroupLimit` all serialize their keys through this serde, so they all fall back, unless `spark.comet.expression.SortOrder.allowIncompatible=true`. Native range partitioning already declines array and struct keys. A key whose type cannot hold a nested null, such as `array(coalesce(x, 0))`, stays native under any null order, since both engines place a null key itself by the null order. The strict floating-point check for nested float keys stays. It still covers RANGE window frames over those keys, which #6477 is about. Once that fix lands too, the strict-only check is redundant and can go, along with the `expect_fallback`s in `nested_float_order_keys_strict.sql`. The operator compatibility guide gains a short Sort section describing the fallback. ## How are these changes tested? The new `operators/sort_nested_null_order.sql` fixture turns off the harness's `SortOrder` opt-in and checks that `ASC NULLS LAST` and `DESC NULLS FIRST` fall back, with Spark's answers, for an array key, a struct key, an array of arrays, a TopK, and `RANK` and `DENSE_RANK` window order keys. It also checks that the default null orders and keys that cannot hold a nested null stay native. With the new branch in `getSupportLevel` disabled, the fixture fails on the first query with Comet's wrong order. On Spark 4.1, these passed: the new fixture, every fixture under `windows/` and `operators/`, `CometWindowExecSuite`, `CometTopKSuite`, `CometFloatSemanticsSuite`, the sort tests in `CometExecSuite`, and the strict floating-point sort tests in `CometExpressionSuite`. On Spark 3.5, the `operators/` and `windows/` fixtures, `CometTopKSuite` and `CometWindowExecSuite` passed. -- 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]
