Skip to content

fix: order nulls inside nested values first in least - #6761

Open
viirya wants to merge 1 commit into
apache:mainfrom
viirya:fix-least-nested-null-ordering
Open

viirya wants to merge 1 commit into
apache:mainfrom
viirya:fix-least-nested-null-ordering

Conversation

@viirya

@viirya viirya commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

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 greatest and least. Comet sent nested types without a float leaf to DataFusion's least, which orders such a null after every other value (nulls_first: false), so least could return a different value from Spark.

What changes are included in this PR?

SparkGreatestLeast::handles now accepts every nested type, not only those with a float leaf. For those types, SparkGreatestLeast compares with spark_comparator, which already orders nested nulls first. Scalar non-float types still use DataFusion's greatest and least.

How are these changes tested?

  • New SQL file expressions/math/greatest_least_nested_nulls.sql, covering structs, arrays, and arrays of structs with a null nested value. It fails without this change: for id = 3, Spark returns [null, 3] and Comet returned [3, 1].
  • New Rust unit test nested_nulls_sort_first; handles_floats_and_nested_types_only replaces the previous handles test.
  • The existing greatest, least, and float SQL files still pass.

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>
@viirya viirya added the run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue label Oct 7, 2026
@github-actions github-actions Bot added bug Something isn't working area:expressions Expression evaluation labels Oct 7, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 least orders nested nulls last, so Comet could return [3, 1] where Spark returns [null, 3].
  • Design approach: Expand SparkGreatestLeast::handles to route every nested result type through the existing Spark-compatible comparator.
  • Correctness: Compared Spark’s interpreted and generated Least/Greatest implementations 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_comparator for 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 zip pipeline. 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 SparkGreatestLeast and spark_comparator interfaces fit the expanded responsibility.
  • Behavioral changes worth calling out: Compared with branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, this intentionally corrects non-float nested least results. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation bug Something isn't working run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

least orders a null inside a struct or array last, where Spark orders it first

2 participants