andygrove opened a new pull request, #6475:
URL: https://github.com/apache/datafusion-comet/pull/6475
## Which issue does this PR close?
Closes #5507.
Part of #6385: the "nested sort keys and rank (#5507)" step.
## Rationale for this change
Spark orders floats with `SQLOrderingUtil.compareDoubles` at every depth of
an array or struct: `-0.0` equals `0.0`, all NaNs are equal, and NaN sorts
above every other value. #5469 made Comet normalize scalar `FLOAT` and `DOUBLE`
sort and window keys so that Arrow's total order agrees with Spark's, but a key
that nests floats in an array or struct was compared raw. There `-0.0` sorts
below `0.0`, and a NaN with the sign bit set sorts below `-Infinity`. Negating
a NaN sets that bit on any platform, and on x86-64 every NaN that arithmetic
produces has it.
With rows holding `-0.0`, `0.0`, a canonical NaN and a sign-bit NaN, among
others:
| Query | Spark | Comet before |
| --- | --- | --- |
| `SELECT id FROM t ORDER BY array(IF(s, -d, d)), id DESC` | `8, 9, 7, 2, 1,
3, 6, 5, 4` | `8, 5, 9, 7, 1, 2, 3, 6, 4` |
The two zeros (ids 1 and 2) and the two NaNs (ids 4 and 5) are peers in
Spark, so the tiebreaker orders them; Comet split them and sorted the sign-bit
NaN first. `RANK`, `DENSE_RANK`, window frames and rank limits over such keys
had the same gap. `spark.comet.exec.strictFloatingPoint=true` made these keys
fall back to Spark, unless `spark.comet.expression.SortOrder.allowIncompatible`
was set.
## What changes are included in this PR?
- `create_normalized_key_expr`, which builds the keys of Sort, TopK, Window
and WindowGroupLimit, wraps an array or struct key with a float at any depth in
`NormalizeNestedFloats`, as it already wrapped a scalar float key in
`NormalizeNaNAndZero`. Only the comparison key is normalized, so the rows keep
their original values, zero signs and NaN payloads included.
- `CometSortOrder` is compatible for every key type, so strict
floating-point mode no longer makes these keys fall back. Maps cannot be sort
keys in Spark.
- Range partitioning needs no change: the native range partitioner only
accepts scalar keys, and a nested key goes to the JVM shuffle, which partitions
with Spark's own ordering.
- The floating-point compatibility guide, the operator compatibility and
tuning guides, the `spark.comet.exec.strictFloatingPoint` description, the
native shuffle contributor guide and the shuffle review skill no longer
describe nested keys as a gap.
## How are these changes tested?
- `windows/nested_float_order_keys.sql` (new, run with strict floating-point
mode off and on, and with `SortOrder.allowIncompatible=false` so strict mode
applies the shipped policy): arrays, structs, arrays of structs and structs of
arrays of `DOUBLE` and `FLOAT` holding both zeros and both kinds of NaN, as
keys of `ORDER BY` in both directions, TopK, `RANK`, `DENSE_RANK`, running sums
over the default `RANGE` frame, and rank limits. Every `ORDER BY` ends with a
unique tiebreaker, so peers must come out in its order. On `main` its first
query returns the wrong order shown above, and in strict mode `main` falls back
to Spark instead.
- Two nested-null differences that do not involve floats stay out of the
fixture: the queries keep the default null orders, and the running sums leave
out the row whose keys hold a null element. Spark orders a null element below
every value whatever `NULLS FIRST` or `NULLS LAST` says, while the native sort
ties nested nulls to that option, and DataFusion's `RANGE` frame bounds order a
null element above every value. Both show up with `INT` keys too, so they need
their own fix.
- The rank-limit unit test
`floating_sort_keys_preserve_window_group_limit_peers` runs its sign-bit,
payload and signaling NaNs, zeros and nulls through a one-element list and a
one-field struct as well as bare, and checks the same peers and the same bits.
It fails at the first list key without the change.
- `CometExpressionSuite`: the two tests that asserted the strict-mode
fallback for array and struct sort keys now check that the sort runs natively
and matches Spark, over data written to Parquet with a unique `id` sorted last.
A new test sorts on `array(d)` and `named_struct('v', d)` over a local relation
and checks that NaN payloads and zero signs come back unchanged, with the zeros
and the two NaNs as peers.
- Results on macOS aarch64 with the default Spark 4.1 profile:
- `CometSqlFileTestSuite`: 593, including both runs of the new fixture.
- `CometExecSuite`: 152. `CometWindowExecSuite`: 67.
`CometExpressionSuite` floating-point tests: 23.
- Unit tests in core: 579.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
