You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2026/08/26 02:58:24 UTC

[PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

sunchao opened a new pull request, #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469

## Why are the changes needed?

Follow-up to #4870. Spark treats all NaN representations as equal and greater than non-NaNs, and treats `-0.0` and `+0.0` as peers. Arrow's raw total ordering distinguishes their bits, so native `RANK` / `DENSE_RANK` cutoffs can drop rows Spark retains. A native regression fixture returns 8 rows in Spark but only 4 without this fix.

Fixing peer equality alone is insufficient: with secondary ordering keys, a sort that distinguishes NaN payloads or zero signs can separate rows that should be peers.

## What changes were proposed in this PR?

- Reuse `NormalizeNaNAndZero` for scalar `FLOAT` / `DOUBLE` sort expressions shared by Sort, Window, and WindowGroupLimit.
- Normalize native range-shuffle boundaries with the same helper as incoming keys.
- Preserve original output values, including NaN payloads and zero signs; normalize only comparison keys.
- Add native and Spark regressions for both float widths, NaN signs/payloads, signed zeros, compound keys, batch boundaries, null ordering, and range routing. Re-enable the existing signed-zero SQL regression.
- Document that nested floating values and the existing strict-floating-point fallback policy are unchanged.

## How was this PR tested?

- `make core`; `cargo fmt --all -- --check`; `cargo clippy --all-targets --workspace -- -D warnings`.
- `cargo test --workspace --lib`: 908 passed, 4 existing ignored tests.
- Spark 4.0: all 73 selected tests passed (full `CometWindowExecSuite`, all WindowGroupLimit SQL cases, and existing sort fallback tests).
- Spark 3.5: all 21 selected tests passed (new regressions, all WindowGroupLimit SQL cases, and existing sort fallback tests).
- Spark 4.1: all 21 selected tests passed (new regressions, all WindowGroupLimit SQL cases, and existing sort fallback tests), including Scala style and formatting checks.
- Negative controls with the planner fix removed: both new native regressions and both Spark 4.0 FLOAT/DOUBLE regressions fail; the fixed runs pass. Spark tests explicitly require native Sort and WindowGroupLimit and check original output bits.

## Which issue does this PR close?

Closes #5468.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469#issuecomment-5457934428

   @comphead Thanks for the pointer. I checked [the SPARK-54918 patch](https://github.com/apache/spark/commit/a4aa4adf4fd0f790d7b71b322292050f6cf8db38). It extends normalization to array set operations such as `array_distinct` and `array_union`.
   
   For the scalar sorting and ranking cases here, Spark already treats `-0.0` and `+0.0` as equal before 4.2. The problem is that Comet’s native comparison can distinguish them, splitting rows that should share a rank and potentially dropping qualifying rows at a rank cutoff. 
   
   This PR normalizes the comparison keys consistently across sorting and ranking, so the fix is still needed with Spark 4.2. 


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

andygrove commented on PR #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469#issuecomment-5441291460

> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

This is a thorough piece of work. Reusing `NormalizeNaNAndZero` rather than inventing a new comparator is the right call, and I like that `create_normalized_key_expr` is shared by sort, window partition keys, window group limit, and range partitioning so the four cannot drift apart. The `floating_window_partition_keys_preserve_ordering` test that checks `get_ordered_partition_by_indices` still returns the prefix is a good catch, since silently losing the sorted window mode would have been an easy regression to miss. Flipping the `ignore(...)` in `window_group_limit_rank.sql` back to a real assertion is nice to see.

A few things I would like resolved before merge.

**No performance data for the sort path**

`create_sort_expr` now wraps every scalar `FLOAT` and `DOUBLE` sort key, which means `native/core/src/execution/planner.rs:1498` adds a full column copy per batch to every native sort on a floating key, plus the same for window ordering and range partitioning. `unary` allocates a fresh values buffer each time. That is probably fine relative to the row encoding and the sort itself, but "probably fine" is not a number. Could you run a sort benchmark with a float key before and after and put the result in the description? If it turns out to be measurable, it would be worth discussing whether the normalization can be folded into the row encoder instead of materializing an intermediate array.

**`strictFloatingPoint` now falls back for cases this PR fixes**

The docs change says the `spark.comet.exec.strictFloatingPoint=true` fallback policy is unchanged, "including its conservative fallback for scalar floating-point sort keys." After this PR those scalar keys agree with Spark, so users who turn strict mode on are paying a fallback for something that is now correct. Is narrowing that fallback to the nested array and struct cases in scope here, or should it be a follow-up? If it is a follow-up I would like an issue filed and linked from this PR, because a stale conservative fallback is the kind of thing that quietly stays for a year.

**Tracking issue for the nested case**

Same question for the nested array and struct divergence the docs now call out. There is no issue link in the new `floating-point.md` text. A user who reads that paragraph and is affected has nowhere to go. Could you file one and link it from the doc?

**Sort-merge join builds an expression it throws away**

At `planner.rs:2145` the `create_sort_expr` result is used only for `sort_expr.options`, and the expression itself is discarded. That call now constructs a `NormalizeNaNAndZero` node for nothing on float keys. It is not a correctness problem, but it does suggest that site wants a smaller helper that only decodes direction and null ordering. Worth splitting out?

**Silent passthrough in `normalize_array`**

`NormalizeNaNAndZero::normalize_array` returns the input untouched for any type that is not `Float32` or `Float64`, including `Dictionary(_, Float64)`, `List<Float64>`, and structs with float fields. In the range-partitioning call site that is consistent with `create_normalized_key_expr`, which also only handles the two scalar types, so I do not think there is a bug here today. But the function is `pub` and the passthrough is silent. Could the doc comment say explicitly that nested and dictionary-encoded floats are deliberately not normalized, and that callers must match `create_normalized_key_expr`'s coverage? Otherwise the next caller gets a silently wrong answer with no compile error.

**One test question**

In `CometWindowExecSuite`, the new tests set `COMET_EXEC_STRICT_FLOATING_POINT` to `false` explicitly, which is already the default. Is that there to document intent, or to guard against the default flipping? If the former, a short comment saying so would help, since otherwise it reads like the tests only pass in non-strict mode.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao merged PR #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469#issuecomment-5442862621

Updated in [38825e26a](https://github.com/apache/datafusion-comet/commit/38825e26a1324297a46821a06fe5ce33e8b47779).

The follow-up adds the sort benchmark and links the two remaining compatibility issues from the guide. I checked the six points separately:

1. **Sort cost:** [the new benchmark](https://github.com/apache/datafusion-comet/blob/38825e26a1324297a46821a06fe5ce33e8b47779/native/spark-expr/benches/sort_float_keys.rs) runs DataFusion 54.1.0 `SortExec` with bare versus normalized keys in the same release binary, using 262,144 rows, 32 batches, one partition/thread, and both finite/non-null and null/NaN/signed-zero inputs. Two passes reverse the variant order. FLOAT median changes were small (−1.7% to +4.7%, with mixed-input direction varying); DOUBLE cost was measurable in both passes: +6.6–11.4% finite and +5.2–12.5% mixed. The description contains both passes and reproduction commands. This is a SortExec benchmark, not an end-to-end Spark result. Peak pool reservations were unchanged, but they exclude temporary normalized arrays: each evaluation adds a 4/8-byte-per-row values buffer and shares validity. Already-normalized keys are reused, so this measures previously bare keys rather than another copy at every call. Fo
lding normalization into sorting/encoding is a worthwhile optimization to investigate separately; this change does not claim the cost is free.

2. **Strict-mode admission:** tracked in [#5506](https://github.com/apache/datafusion-comet/issues/5506), now linked from the guide. The follow-up is scoped to scalar Sort admission with strict-mode execution tests; this PR retains the existing guards for other consumers and nested keys.

3. **Nested ordering/rank:** tracked in [#5507](https://github.com/apache/datafusion-comet/issues/5507), also linked from the guide. Diagnostic SQL reproduced array/struct ORDER BY and array RANK differences for FLOAT and DOUBLE with native operators asserted. Struct RANK already falls back in those probes; it is not a reproduced native wrong result. Strict-mode controls matched Spark. The issue records the reused-native-build limitation, so these are evidence for the remaining limitation, not a fresh full-head validation claim.

4. **Sort-merge-join options:** the observation is correct, but that site constructs and discards a planning-time expression. It does not evaluate the normalizer or copy a column there; it already built an expression just to obtain options before this PR. I left the optional helper cleanup out of this correctness fix.

5. **`normalize_array` coverage:** the existing doc comment specifies scalar floating comparison keys and says other types are unchanged. Its Float32/Float64 match and `create_normalized_key_expr` use the same gate: dictionary, list, and struct arrays deliberately pass through. There is no recursive or dictionary-normalization promise in that API; the nested limitation now has the explicit tracker above.

6. **Explicit `strictFloatingPoint=false` in tests:** it pins the native execution path under test, independently of externally supplied configuration or a future default change. The tests assert native Sort/WindowGroupLimit operators; `true` deliberately still invokes the conservative fallback tracked by #5506. I kept the setting for that reason.

The new release benchmark built, all eight smoke cases passed, and benchmark Clippy (`-D warnings`), formatting, and diff checks passed. I also corrected the stale validation paragraph: the earlier full-suite results are historical, not rerun for this benchmark/documentation-only follow-up.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: normalize scalar float sort and window rank keys [datafusion-comet]

Posted by "comphead (via GitHub)" <gi...@apache.org>.

comphead commented on PR #5469:
URL: https://github.com/apache/datafusion-comet/pull/5469#issuecomment-5454294296

   One thing to consider is https://issues.apache.org/jira/browse/SPARK-54918 which fixes +0, -0 in Spark 4.2.0


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org