Conversation
`CometSortOrder.getSupportLevel` delegated to the recursive `SupportLevel.strictFloatingPointReason`, which rejects a data type that contains a float or double at any nesting level. Under `spark.comet.exec.strictFloatingPoint=true` that rejected scalar FLOAT and DOUBLE sort keys as well as nested ones. Scalar floating-point comparison keys are normalized before native Sort, TopK, Window, WindowGroupLimit, and range partitioning (apache#5469): NaN payloads are folded together and signed zeros tied, matching Spark's SQLOrderingUtil.compareDoubles/compareFloats. Only the comparison key is normalized, so returned values keep their original NaN representation and zero sign. Those keys are therefore compatible even in strict mode. Match scalar FLOAT/DOUBLE ahead of the shared helper so they report Compatible, and leave every other data type on the existing recursive path. Floats nested in arrays, structs, and maps still sort by Arrow's raw total ordering, under which -0.0 sorts below 0.0 and a sign-bit NaN sorts below -Infinity, so they keep falling back (apache#5507). The three other callers of `strictFloatingPointReason` are untouched, including `SortArray`, so `sort_array` on a floating-point element type still falls back. The description of `spark.comet.exec.strictFloatingPoint` is updated to say which sorts are still affected. It is scraped into the generated config reference, so it is the only warning many users will read. The subject of the fallback reason is shared with `getIncompatibleReasons()` through one private val, since that text is published per Spark version in the generated compatibility pages and the two must not drift. Tests: the existing scalar test now asserts native execution instead of fallback, and sorts on a unique id last. -0.0 and +0.0 are peers under Spark's comparison, so ordering on the float columns alone leaves ties whose relative order neither engine promises, and Comet and Spark already lay those out differently today with strict mode off. The sibling array and struct tests still assert the strict-mode fallback. A generated matrix covers both types, both directions, both null orderings and compound keys, one test pins that the sort leaves NaN payloads and zero signs untouched, and one covers the TopK path, which reaches the same gate but a different operator.
`supportedRangePartitioningDataType` rejected FLOAT and DOUBLE whenever `spark.comet.exec.strictFloatingPoint=true`. This is a second admission gate, independent of `CometSortOrder`, so narrowing only the sort gate left a plan that sorts natively but still shuffles on the JVM: `CometSort` over `CometColumnarExchange` rather than `CometExchange`/`CometNativeShuffle`. The native range partitioner normalizes both sides of its comparison. Incoming keys are the serialized sort-order expressions, which carry the `NormalizeNaNAndZero` wrapper, and the sampled boundary rows are normalized before being row-encoded with the same converter. Scalar floating-point keys therefore partition consistently with Spark's ordering, so the strict-mode rejection buys no correctness. Accept scalar FLOAT and DOUBLE, and drop the now-unreachable strict-mode fallback reason. Nested types are unaffected: they already fall through to the catch-all that rejects them. The two policy tests are merged into one parameterized over strict mode, since the expected outcome no longer depends on it. The merged test also drops `SortOrder.allowIncompatible=true`, which the old strict-mode test needed to reach this gate, so it now proves the whole path works without an escape hatch.
`CometSortOrder` is the admission gate for nine serde sites, so narrowing it for scalar floating point opens more than `ORDER BY`. Each of these was verified rather than assumed. Sort-merge join synthesizes sort orders and therefore reaches the same gate, which means strict mode used to make FP-keyed joins fall back as a side effect. Comet does not normalize the join keys itself. It compares `join_on`, and correctness rests on Catalyst's NormalizeFloatingNumbers having already wrapped both sides, so the new test pins that dependency. Its fixtures are local relations rather than Parquet tables because a Parquet round trip canonicalizes every NaN payload, which would collapse the two NaN cases into one. The WindowGroupLimit peer test is parameterized over strict mode, and a second window test is added that is deliberately not gated on Spark 3.5, so window ordering has coverage on every supported version. It orders by a floating point column and sums over the default RANGE frame, which spans the peer group, so -0.0 and +0.0 being peers is visible in the result and the expected values do not depend on tie order. Columnar shuffle performs range partitioning on the JVM with Spark's own RangePartitioner, but still probes whether Comet can serialize the sort order, so strict mode used to abandon an exchange it would have executed identically. The new test pins the floating-point half of that. Consulting a native-serde gate on a path that never goes native is tracked in apache#5971.
The floating-point compatibility page still carried apache#5469's note that the strict policy was unchanged for scalar sort keys, pointing at apache#5506. The tuning page stated flatly that sorting on floating-point data is not compatible with Spark, which was already wrong for scalar keys after apache#5469. Both now scope the claim to ordering keys. `ORDER BY`, window ordering and range partitioning on scalar FLOAT and DOUBLE stay native under strict mode. Values nested in arrays, structs and maps still fall back, and so does `sort_array` on a floating-point element type, which sorts elements rather than ordering rows and is gated separately. Neither page is generated. `GenerateDocs` emits the per-expression compatibility pages and the config reference, so those pick up the source changes in this branch on their own.
github-actions
Bot
added
enhancement
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This removes an admission restriction left after native floating-point comparison keys were normalized. At 9fcfbb636da5335f1c57ef9a3b755dfb9443229c, scalar FLOAT/DOUBLE sort orders and native range-partitioning keys can run with strict floating point enabled and SortOrder.allowIncompatible=false. Nested floating-point sort keys still receive the strict-mode incompatibility result, native range partitioning still rejects complex keys, and sort_array retains its existing strict-mode handling. Child-expression conversion and the other operator/type gates remain in place.
The behavior matches the maintained Spark 3.5 and 4.0 comparators: signed zeros are equal, all NaNs are equal and greater than non-NaNs, null placement is independent of sort direction, and later keys break ties. I traced Sort/TopK, Window, WindowGroupLimit and range partitioning through the existing native normalization helper. It wraps the evaluated scalar key, and range boundaries receive the same normalization. The operators retain the original output columns. Spark's optimizer separately normalizes equijoin keys, and Comet preserves that expression, covering the newly admitted sort-merge-join path. No verified P1/P2 issue found.
The added tests explicitly disable the incompatible override and assert native operators or shuffle modes. They cover FLOAT/DOUBLE, ascending/descending order, null placement, compound keys, TopK, signed-zero window peers and both shuffle implementations. Local-relation tests preserve distinct NaN payloads and signed-zero bits when checking returned values and WindowGroupLimit peers.
Independent validation compiled the exact admission helper bodies with minimal type/config stubs and passed 86 checks. A second component probe compiled the production normalization body and locked Arrow 59.3.0 float-encoding bodies, then compared them with canonical Spark SQLOrderingUtil: 163,592 ordering comparisons, 163,592 peer-equality checks and 163,592 compound-key checks passed. The unnormalized negative control produced 4,528 mismatches. The nullable-encoding and compound-key wrappers are harness models. These are component checks, not execution of Spark/JNI or the full Arrow/DataFusion operators. I did not run the product suites or independently reproduce the author's reported suite results. Required maintained Spark 3.4/4.1 branches were unavailable, so those versions remain a source-coverage limitation.
At the 2026-09-16T10:42:58.087073+00:00 public cutoff, CI, CodeQL and Check PR Title were action_required, each with zero jobs. Only the label check had succeeded. The head and merge were unchanged. The authored ten-file patch from merge base 41d6d448 is preserved exactly in the effective base-to-merge changes. The two newer, unrelated base commits are not regressions introduced by this PR.
Performance
The change reuses the existing normalization and execution paths. It does not add a new per-row kernel or another copy of the returned columns. Strict mode can now select native sorting and range shuffling where it previously blocked them. Normalized comparison arrays are still required for Spark-compatible NaN and zero behavior, and the planner avoids adding another wrapper when the key is already normalized.
The author reports a same-build strict-mode ablation using SortOrder.enabled=false for the baseline, with a roughly 1.7x improvement on a four-million-row DOUBLE sort and reverse-order runs. I have not independently reproduced that result and do not treat it as a general workload guarantee. The existing native sort benchmark also exercises normalized keys. I found no additional material cost introduced by the admission change.
Design
The implementation places the exception at the two admission boundaries that were still blocking a supported scalar operation: CometSortOrder.getSupportLevel and the native range-partitioning type predicate. Keeping the native comparison implementation unchanged makes the compatibility argument straightforward and avoids broadening the policy for nested values. The config and user documentation describe the resulting distinction, and the new caller-specific tests cover paths beyond a standalone ORDER BY.
The source and tests support the proposed boundary. No design change is requested.
Abstraction & complexity
No new abstraction or execution mode is introduced. The scalar cases are explicit, the shared recursive strict-mode helper continues to own nested-type policy, and the range predicate removes an obsolete scalar-only rejection. Sharing the nested incompatibility description between admission feedback and generated documentation reduces the risk of policy text drifting. The implementation is appropriately small for the behavior change.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 9fcfbb63 after CI execution became available. The head, base and ten-file contribution are unchanged from the approved review. The failing file is byte-identical at the head and the CI merge checkout f445979d.
There is one new P2 CI finding: Scala syntactic lint exits 32 because the new range-partitioning test has a redundant s prefix. The inline comment identifies the one-character fix. CodeQL and the title check now pass, while nine build/test checks are running as of September 16, 14:35 UTC. No additional semantic finding, local product run or benchmark result is added. Preserving the existing approval while reporting this CI failure.
| // not strict floating point is on. Neither gate needs the allowIncompatible escape hatch. | ||
| Seq("true", "false").foreach { strict => | ||
| test( | ||
| s"range partitioning on floating-point uses native shuffle when " + |
There was a problem hiding this comment.
Correctness
[P2] Remove the redundant interpolation prefix to unblock CI
Could you remove the s from this first test-name string, keeping it on s"strictFloatingPoint=$strict"? The current CI lint job runs Scalafix 0.14.6 with RedundantSyntax and exits 32 with this exact change as its expected fix. The file is identical at this head and the job's merge checkout f445979d. The Linux workflow feeds Required Checks, so this failure prevents that aggregate check from passing.
2 participants
Which issue does this PR close?
Closes #5506.
Rationale for this change
#5469 normalized scalar floating-point comparison keys natively, so they match Spark's
SQLOrderingUtil. Admission did not follow. Underspark.comet.exec.strictFloatingPoint=true, two gates still rejected scalarFLOAT/DOUBLEkeys, so the whole stage fell back to Spark:On 4M
DOUBLErows with negative zeros and NaNs,ORDER BY v, idwent from a median of 4400 ms to 2551 ms (1.7x). The "before" plan is reproduced on the same build withspark.comet.expression.SortOrder.enabled=false, and the result held with the run order reversed.What changes are included in this PR?
CometSortOrder.getSupportLevelreports scalarFloatType/DoubleTypeasCompatible. Floats nested in arrays, structs and maps still fall back (Match Spark ordering and rank semantics for floating values nested in arrays and structs #5507). The other callers ofstrictFloatingPointReason, includingSortArray, are unchanged.supportedRangePartitioningDataTypeaccepts scalar float and double. The native range partitioner already normalizes both its keys and its sampled bounds.strictFloatingPointconfig description,tuning.mdandfloating-point.mdnow say which sorts are still affected, includingsort_array.How are these changes tested?
CometSortExecand equality with Spark. Separate tests cover TopK, raw-bit fidelity of returned values, window ordering on every Spark version, sort-merge join, and columnar shuffle.SortOrder.allowIncompatible=false, becauseCometTestBasedefaults it to true.CometSqlFileTestSuite. Spark 3.4/4.0 and the TPC-DS plan stability suites, which needSPARK_HOME, are left to CI.