andygrove opened a new pull request, #7114:
URL: https://github.com/apache/datafusion-comet/pull/7114
## Which issue does this PR close?
Closes #6636.
Part of #6615 and #6385.
## Rationale for this change
`CometFloatSemanticsSuite` crossed Spark's `-0.0` and NaN edge values with
every operator context and float expression, as 412 runtime tests generated by
one `test(c.name)` call. It only called `checkSparkAnswer`, so a case that fell
back to Spark passed without exercising Comet. #6615 moves such sweeps to SQL
file tests.
#6636 expected most of the 412 cases to be known gaps that the SQL framework
cannot hold. #6447, #6457, #6475 and #6518 closed those gaps: the one that
remains is `collect_set` before Spark 4.2 (#5312).
## What changes are included in this PR?
- The matching cases move to `sql-tests/float_semantics/`, 400 queries over
`DOUBLE` and `FLOAT` in seven files.
| File | Queries |
|---|---|
| `comparison.sql` | 112: `Project`, `Filter`, aggregate argument and
`FILTER`, sort key, `Generate`, window argument and partition key |
| `comparison_joins.sql` | 56: broadcast hash, shuffled hash, sort merge
and nested loop join conditions |
| `comparison_nested.sql` | 56: array and struct operands |
| `literal_comparison.sql` | 42: a column and each edge literal on either
side, in `Project`, `Filter` and a Parquet scan filter |
| `keys.sql` | 40: grouping, join, set, `IN`, sort and window keys |
| `expressions.sql` | 68 |
| `aggregates.sql` | 24 |
| `aggregates_collect_set_4_2.sql` | 2: `collect_set`, `MinSparkVersion:
4.2` |
Each file builds its tables from the nine edge values and a flag. Spark's
Parquet writer canonicalizes NaN, so the sign-bit NaN is made at query time by
negating the stored NaN, in a CTE that replaces the suite's temporary views.
The files turn `ConstantFolding` on, as the sweep always ran, because a literal
such as `-CAST('NaN' AS DOUBLE)` has to fold for the sign-bit NaN to reach
Comet as a constant and for the Parquet scan filter to be pushed down.
- A plain `query` asserts native execution. 392 of the 400 do. The other 8
are `query spark_answer_only`, each with the reason above it: `mode` (opt-in,
`mode.sql` runs it natively), `array_distinct` and `array_union` (native only
on some Spark patch releases, `array_distinct.sql` and `array_union.sql`) and
the map lookup (a float key falls back, `map_builders_float_key_gate.sql`).
- 12 cases are not moved, because existing fixtures hold the same cases over
the same edge values: `hash`, `xxhash64` and their array and struct forms
(`expressions/hash/hash.sql`), and the nested `ORDER BY` and window order keys
(`windows/nested_float_order_keys.sql`).
- `CometFloatSemanticsSuite` shrinks to the one known gap. A SQL file test
has no mode that expects a difference, so `collect_set` before Spark 4.2 stays
a strict Scala test that fails once the answers match, and
`aggregates_collect_set_4_2.sql` covers 4.2 and later. The test removes itself
when #5312 lands.
- `contributor-guide/floating_point.md` points a new expression at the
fixtures.
## How are these changes tested?
The fixtures are the tests. `CometSqlFileTestSuite float_semantics` passes
on Spark 3.4, 3.5, 4.0, 4.1 and 4.2 (8 tests each;
`aggregates_collect_set_4_2.sql` only runs on 4.2), and
`CometFloatSemanticsSuite` passes on 4.1. The seven files take about a minute
together on 4.1.
I checked that the converted sweep still fails when the native helpers are
wrong, by mutating `native/spark-expr/src/float_semantics/mod.rs` and running
the fixtures on 4.1:
| Mutation | Files that fail |
|---|---|
| `normalize_float` stops canonicalizing NaN | `expressions`, `aggregates`,
`comparison_joins`, `keys` |
| `normalize_float` stops folding `-0.0` | `expressions`, `aggregates`,
`keys` |
| `compare_floats` stops treating two NaNs as equal | `expressions`,
`comparison_nested` |
Join conditions are planned as intended (`CometBroadcastHashJoin`,
`CometHashJoin`, `CometSortMergeJoin`, `CometBroadcastNestedLoopJoin`); the `=`
and `<=>` cases of the nested loop context become hash joins, as they did in
the suite.
The change adds no production code, but it touches every Spark profile's
fixtures, so it wants `run-all-spark-profiles`.
--
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]
