Repository navigation
Conversation
Spark orders a null inside a struct or array before every other value, in both greatest and least. DataFusion's least orders it after every other value. Route every nested type to SparkGreatestLeast, whose comparator orders nested nulls first, instead of only types with a float leaf. Co-authored-by: Isaac <no-reply@databricks.com>
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full three-file, one-commit diff at 00ec7d2799b46d3a40158b53cfbc1fd355dde7e4 against b56349697b786ff2ad1c1bcf6ecf45b809af5f30. The PR is not a draft. Applied review-comet-pr and review-comet-expression-pr, with the relevant contributor guidance. The snapshot and live discussion checks contained no existing reviews, conversation comments, inline comments, or review threads.
Summary
- Prior state and problem: Nested values without float leaves used DataFusion’s extrema functions. Its
leastorders nested nulls last, so Comet could return[3, 1]where Spark returns[null, 3]. - Design approach: Expand
SparkGreatestLeast::handlesto route every nested result type through the existing Spark-compatible comparator. - Correctness: Compared Spark’s interpreted and generated
Least/Greatestimplementations across supported 3.4–4.2 versions, including array and struct ordering. The implementation preserves top-level null skipping, nested nulls-first ordering, lexicographic comparison, and first-argument tie handling. Focused tests reproduced the original failure through base routing and passed at the reviewed head. - Compatibility analysis: The relevant Spark semantics are consistent across the supported versions. Existing recursive collation checks remain in place. No protobuf, configuration, shim, or return-type contract changes were introduced.
- Key design decisions: Keep scalar non-float inputs on DataFusion’s existing path and reuse
spark_comparatorfor nested inputs. Existing floating-point handling remains unchanged. - Implementation sketch: The production change broadens the routing predicate and updates its explanatory comments. A Rust regression test, updated routing assertions, and three column-dependent SQL queries cover structs, arrays, and arrays of structs.
- Performance: Scalar non-float execution is unaffected. Newly routed nested inputs use the existing argument normalization, comparator, selection bitmap, and Arrow
zippipeline. Performance assessment was source-based. No benchmark was run, and no evidence-backed P1/P2 performance regression was identified. - Design: Reusing the established comparison path keeps nested ordering consistent without duplicating comparator logic. The distinction between skipping null arguments and ordering null children is clear.
- Abstraction & complexity: No new abstraction or configuration is added. The existing
SparkGreatestLeastandspark_comparatorinterfaces fit the expanded responsibility. - Behavioral changes worth calling out: Compared with
branch-1.1ate9efd9f764ee0a59b7898ff028d6985d4a7a28e1, this intentionally corrects non-float nestedleastresults. Earlier floating-point corrections already present in the supplied base are separate from this PR. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified. The running CI jobs still need to establish the integration-test verdict.
Validation: A standalone harness compiled the exact-head expression and comparator modules against cached Arrow 59.3.0 and DataFusion 55.1.0, matching Cargo.lock. All 26 source unit tests and three additional review tests passed. Additional checks covered 1,728 list triples, mixed scalar/column arguments, null and empty values, sliced batches, structs, and arrays of structs. The same additional tests failed through base routing because of the known nested-null ordering bug. git diff --check passed and the checkout remained unchanged.
Exact-head CI: Seven checks succeeded, fourteen were skipped, and two remained in progress. No failed checks were reported. The active jobs were the main CI preflight and the labeled Spark 4.1 native/JVM build in runs https://github.com/apache/datafusion-comet/actions/runs/37680375961 and https://github.com/apache/datafusion-comet/actions/runs/37680397841. This is not a completed integration-test verdict. I did not run a full native/Maven build or the Comet SQL suite locally.
No introduced P1/P2 issues found within this review. No substantiated existing blockers remain.
Which issue does this PR close?
Closes #6759.
Rationale for this change
Spark orders a null inside a struct or array before every other value, in both
greatestandleast. Comet sent nested types without a float leaf to DataFusion'sleast, which orders such a null after every other value (nulls_first: false), soleastcould return a different value from Spark.What changes are included in this PR?
SparkGreatestLeast::handlesnow accepts every nested type, not only those with a float leaf. For those types,SparkGreatestLeastcompares withspark_comparator, which already orders nested nulls first. Scalar non-float types still use DataFusion'sgreatestandleast.How are these changes tested?
expressions/math/greatest_least_nested_nulls.sql, covering structs, arrays, and arrays of structs with a null nested value. It fails without this change: forid = 3, Spark returns[null, 3]and Comet returned[3, 1].nested_nulls_sort_first;handles_floats_and_nested_types_onlyreplaces the previoushandlestest.greatest,least, and float SQL files still pass.