Repository navigation
Conversation
github-actions
Bot
added
bug
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed all 15 changed files and both commits against base 7d8d9b1544b734c85a93cca2cc9d5380ad70accd, at full head 1722ba3a41980f2b481ffd794ba4b9d4fe3d6b81. The PR remains non-draft. The snapshot and live discussion contained no reviews, conversation comments, inline comments, or review threads.
Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-shuffle-pr for the shared sort-order path and range-partitioning checks.
Summary
- Prior state and problem: Nullable nested keys with
ASC NULLS LASTorDESC NULLS FIRSTfell back because native sorting applied root null placement recursively. Defaultarray_sortused the JVM dispatcher. - Design approach: Wrap affected deterministic keys as
struct(isnull(key), key)and use direction-default null placement. Route recognized defaultarray_sortcomparators to a native function using the existing recursive Spark comparator. - Correctness: Compared Spark’s
ArraySort, root ordering, and recursive array/struct comparison implementations across 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. The rewrite preserves their null-ordering distinction. Focused differential tests passed for sorting, TopK, ranking, window-group limits, and array sorting. No introduced P1/P2 correctness issue was identified. - Compatibility analysis: Custom comparators and nondefault string collations retain the dispatcher path. Boundary-searching
RANGEframes retain their fallback. Native range partitioning still rejects nested keys, preventing a mismatch with unwrapped sampled boundaries. Compared relevant paths withbranch-1.1at8ac6fccde53d96fec22184be287747eeae481cac. Native default array sorting and corrected nested-null ordering are intended changes. Nested-float normalization already existed on the PR base. - Key design decisions: The determinism guard prevents inconsistent double evaluation. Comparator recognition requires the matching expression tree, and the collation check walks nested types. Existing window-frame safeguards remain intact.
- Implementation sketch: Scala serializes the wrapper and adjusted sort options. Rust registers
SparkArraySort, places null elements last, and reuses stable index sorting plus Arrowtake. Tests and compatibility/tuning documentation cover the changed behavior. - Performance: The wrapper adds a boolean field and can evaluate its child twice. The new array path allocates permutation indices and reordered values while avoiding JVM dispatch for recognized comparators. No benchmark establishes the net speedup or a regression, so neither is claimed.
- Design: Separating root null placement from recursive comparison is straightforward and applies consistently through the shared sort serde. The changes remain localized, with existing fallbacks protecting unsupported cases.
- Abstraction & complexity: Reusing
spark_comparatorandsort_byavoids duplicating recursive ordering logic. The small native UDF and targeted serde rewrite are appropriate abstractions. No concrete simplification met the P1/P2 bar. - Behavioral changes worth calling out: Previously rejected deterministic nested-null sort keys can now execute natively without the incompatibility opt-in. Recognized default
array_sortcalls can execute with JVM dispatch disabled. Nondeterministic affected sort keys remain on Spark. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.
Exact-head CI: 26 checks succeeded, 18 were skipped, and none failed. Successful checks include Linux Spark 4.1 execution, expression, shuffle and scan suites, Rust tests, TPC-H/TPC-DS verification, and compilation/lint checks across supported Spark profiles. The Spark SQL matrix, Iceberg suites, macOS build, and benchmark check were skipped. The exact-head CI reports explicitly show the new regression tests passing. Main CI run: https://github.com/apache/datafusion-comet/actions/runs/38527984777
Local validation: Built the pinned JVM sources with Spark 4.1.3/JDK 17 against the native artifact from that exact-head CI run. All six targeted PR tests passed. A disposable 16-query differential fixture also passed, covering all direction/null-order combinations, deeper nesting, root and interior nulls, empty arrays, NaNs, signed zeros, ranking, ROWS windows, strings, and decimals. The fixture was removed and the checkout is clean.
Validation limits: Other Spark versions received source comparison, not local runtime testing. No local Rust rebuild, full upstream Spark SQL/Iceberg run, or performance benchmark was performed. Local formatting checks were skipped during validation; CI formatting checks passed.
No introduced P1/P2 issues found within this review.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
struct(isnull(child), child)duplicates the child expression, which can force… · New
What changed in this PR
This PR updates Comet’s native sorting and array_sort handling to match Spark’s nested-null ordering semantics while keeping more cases on the native path.
Changes:
- Preserve Spark’s nested-null comparison while applying
NULLS FIRST/LASTonly to the root sort key by wrapping certain keys asstruct(isnull(key), key). - Add a native implementation for the default
array_sortcomparator (spark_array_sort) and route eligiblearray_sortcalls to it. - Extend SQL fixtures, Scala test suites, and user docs to reflect the new native behavior and remaining fallbacks (e.g.,
RANGEpeer-boundary search).
| File | Description |
|---|---|
| spark/src/test/scala/org/apache/comet/exec/CometWindowExecSuite.scala | Adds regression coverage for WindowGroupLimit over nested nullable keys with non-default root null ordering. |
| spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala | Adds tests ensuring native sort/window ordering matches Spark’s nested-null semantics; adds native array_sort test coverage. |
| spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala | Adds coverage ensuring collated-string array_sort stays on Spark/JVM path. |
| spark/src/test/resources/sql-tests/windows/nested_float_order_keys_strict.sql | Updates expectations/documentation in SQL tests to reflect newly-native non-default root null ordering (while keeping RANGE fallback). |
| spark/src/test/resources/sql-tests/windows/nested_float_order_keys.sql | Updates test commentary to reflect new native behavior. |
| spark/src/test/resources/sql-tests/operators/sort_nested_null_order.sql | Updates SQL fixture from “expect_fallback” to native execution for affected nested-null sort cases; adds root-vs-interior null placement checks. |
| spark/src/main/scala/org/apache/spark/sql/comet/CometWindowGroupLimitExec.scala | Updates internal notes to reflect new SortOrder handling for nested nulls. |
| spark/src/main/scala/org/apache/comet/serde/arrays.scala | Detects Spark default array_sort comparator and converts it to a new native scalar function when eligible. |
| spark/src/main/scala/org/apache/comet/serde/CometSortOrder.scala | Implements the key-wrapping strategy and updates compatibility gating for nondeterministic nested keys. |
| native/spark-expr/src/comet_scalar_funcs.rs | Registers the new SparkArraySort scalar UDF. |
| native/spark-expr/src/array_funcs/sort_array.rs | Implements spark_array_sort with Spark nested comparator semantics + null-elements-last policy; adds Rust test. |
| native/spark-expr/src/array_funcs/mod.rs | Exports SparkArraySort. |
| docs/source/user-guide/latest/tuning/operators.md | Updates operator tuning docs for nested-null sort behavior and native default array_sort. |
| docs/source/user-guide/latest/compatibility/operators.md | Updates compatibility docs for Sort behavior with nested nulls and remaining RANGE fallback. |
| docs/source/user-guide/latest/compatibility/floating-point.md | Updates floating-point compatibility docs to reflect the new nested-null sorting behavior. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| inside nested arrays and structs below non-null values. A custom `array_sort` comparator continues through the JVM | ||
| codegen dispatcher. |
There was a problem hiding this comment.
Added the non-default collation case in 1172265.
| "Sorting with `ASC NULLS LAST` or `DESC NULLS FIRST` on an array or struct that can hold a " + | ||
| "null element or field" | ||
| private val nondeterministicNestedKeyReason = | ||
| "A nested sort key with nondefault root null placement must be deterministic" |
There was a problem hiding this comment.
The reason now explains the double evaluation.
| val wrapped = if (needsRootNullWrapper(expr)) { | ||
| CreateNamedStruct( | ||
| Seq(Literal("root_null"), IsNull(expr.child), Literal("value"), expr.child)) | ||
| } else { | ||
| expr.child | ||
| } |
There was a problem hiding this comment.
Agreed, the key can be evaluated twice; the determinism guard keeps results consistent. A dedicated node or CSE is a broader change, so I'd leave it for a follow-up.
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
The root-versus-interior null split matches Spark. Comments on docs, redundant test config, test placement and one native hot spot.
| object CometArrayAggregate extends CometCodegenDispatch[ArrayAggregate] | ||
|
|
||
| object CometArraySort extends CometCodegenDispatch[ArraySort] | ||
| object CometArraySort extends CometCodegenDispatch[ArraySort] with CometTypeShim { |
There was a problem hiding this 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?
There was a problem hiding this comment.
Done in 1172265. CometArraySort now extends CometExpressionSerde like CometArrayFilter, so array_sort is listed as Native, with a note on custom comparators and non-default collations.
| -- 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 |
There was a problem hiding this 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.
There was a problem hiding this comment.
Right, nothing reads it anymore. Removed every remaining setting, including both SQL Config lines and the CometTestBase override.
| } | ||
| } | ||
|
|
||
| test("nested sort keys keep root and interior null ordering separate in native") { |
There was a problem hiding this 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.
There was a problem hiding this 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.
| 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)) { |
There was a problem hiding this 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.
There was a problem hiding this 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.
Weijun-H
added
the
run-spark-4.1-tests
Which issue does this PR close?
Follow-up to #6476. The existing fallback fixed its incorrect results; this change runs those sort keys natively. RANGE frames that search boundaries over nullable nested keys continue to use Spark as in #6477.
Rationale for this change
Spark applies
NULLS FIRSTorNULLS LASTto the root sort key. It compares null elements inside arrays and null fields inside structs according to the sort direction. Native sorting applied the root null choice recursively, so the existing safeguard sentASC NULLS LASTandDESC NULLS FIRSTon nullable nested keys to Spark.The default
array_sortcomparator has a related distinction: null elements go last while nulls inside nested values compare below non-null values. It currently uses the JVM codegen dispatcher even though Comet already has Spark's recursive comparator.What changes are included in this PR?
struct(isnull(key), key), using the sort direction's default null placement for nested comparisons. Keep nondeterministic keys on Spark because wrapping would evaluate the key twice.array_sortthrough a native function that uses the existing Spark comparator for nested values and puts null elements last. Custom comparators and arrays with nondefault string collations continue through the JVM dispatcher.How are these changes tested?
make core./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometExpressionSuite nested sort keys keep root"(Spark 4.1, JDK 17)./mvnw test -Pspark-3.5 -Dtest=none -Dsuites="org.apache.comet.CometExpressionSuite nested sort keys keep root"(Spark 3.5, JDK 17)./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometExpressionSuite default array_sort orders null elements last and nested nulls first in native"(Spark 4.1, JDK 17, with JVM codegen dispatcher disabled in the test)./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometArrayExpressionSuite default array_sort with collated strings keeps Spark comparison"(Spark 4.1, JDK 17, flat and nested collated strings)./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite sort_nested_null_order"(Spark 4.1, JDK 17)./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite nested_float_order_keys_strict"(Spark 4.1, JDK 17)./mvnw test -Dtest=none -Dsuites="org.apache.comet.exec.CometWindowExecSuite window group limit: nested null peers with non-default root null ordering"(Spark 4.1, JDK 17)cargo test -p datafusion-comet-spark-expr default_array_sort_puts_null_elements_last_and_keeps_null_rowscargo clippy -p datafusion-comet-spark-expr --lib -- -D warningscargo fmt --all --check,./mvnw spotless:check -DskipTests,git diff --check