You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/10/05 17:29:18 UTC
[PR] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
andygrove opened a new pull request, #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691
**Stacked on #6447.** Review 494af960d; I'll rebase onto main and mark this ready once #6447 lands.
## Which issue does this PR close?
Closes #6590.
## Rationale for this change
Since #6447, every native comparison of `FLOAT` or `DOUBLE` operands follows Spark's SQL ordering, in which `-0.0` equals `0.0`, all NaNs are equal and NaN sorts above every other value. It gets there by normalizing each operand that isn't a literal into a copy and running Arrow's kernels on the copies, which made a comparison slower than DataFusion's own. For arrays and structs, `<=>`, `<`, `<=`, `>` and `>=` deep-copy both operands in every batch, while nested `=` and `<>` already compare in place. The issue has the details.
## What changes are included in this PR?
`spark_comparison` now builds a `SparkComparison` for Float32 or Float64 operands of the same type, and for `<`, `<=`, `>`, `>=`, `<=>` and `IS DISTINCT FROM` on lists and structs with a float leaf. It compares the operands as they are:
- **Flat floats** go through new kernels in `float_semantics/kernels.rs`, column against column or against a scalar. Each operator is one branch-free test over the two value buffers in the style of `float_gt` (`(a > b) | (a.is_nan() & !b.is_nan())`), and the loops pack 64 results into a word at a time. Against a scalar, whether the scalar is NaN picks the test before the loop, so most operators come down to a single IEEE comparison. SQL null rules, including `<=>` and `IS DISTINCT FROM`, are applied with bitmap operations afterwards.
- **Lists and structs** use `spark_comparator` for the ordering operators and `spark_equality` for the null-safe ones, so `<=>` keeps the length check that tells lists of different lengths apart without comparing their elements. When either side has nulls, the comparison walks only the rows where neither side is null, through the validity bitmap, instead of testing both null buffers on every row. Nested `=`, `<>` and `IN` (`NestedPredicate`) get the same treatment.
Planning doesn't change. A literal is still normalized while the plan is built. `FloatOperands::Raw` still leaves a float column compared with a literal to a plain `BinaryExpr` in a scan's data filters, so Parquet pruning recognizes it. Operands of other types, such as dictionary-encoded floats, keep the normalize-then-compare path. An operand whose runtime layout doesn't match its type, which the planner never produces, falls back to normalizing and comparing with Arrow rather than failing.
The floating-point compatibility page no longer says the operands are normalized. The `float_comparison` bench now covers `<`, `=` and `<=>`, and runs a `normalized` engine that builds #6447's expression, so the two can be compared within one run. The `nested_comparison` bench gains `lt` and `not_distinct` modes.
## How are these changes tested?
- **Rust unit tests:**
- The kernels are checked against `compare_floats` on every pair of edge values (both zeros, canonical NaN, a sign-bit NaN, a NaN with a payload, ±1, ±Infinity and NULL), for every operator, Float32 and Float64, column against column and against a scalar. The inputs span whole and partial words and are sliced at several offsets.
- Through `spark_comparison`, sliced columns are compared with scalars that only appear at evaluation time, so planning can't normalize them, as well as scalar against scalar.
- Nested comparisons are tested against list and struct literals on either side, against a null literal and from an offset.
- Further tests cover the null-skipping comparator and the layout fallback.
- The existing `float_operands_follow_spark_ordering` and `nested_float_operands_follow_spark_ordering` now run on the new path.
- Dropping the NaN term from `<=` or from `>` fails four of these tests.
- **Differential test (not committed):** a throwaway test compared the new path with #6447's normalize-then-compare on randomized inputs, with identical results in every case:
- 19,200 flat comparisons: arrays of up to 1,000 rows, Float32 and Float64, sliced, with nulls, all eight operators, and column, literal and runtime-scalar operands.
- 12,000 nested comparisons: lists, structs, lists of lists and lists of structs, with outer and inner nulls and values under null slots.
- **Rust suites:** all 1,132 `spark-expr` tests and the 55 core planner tests pass, workspace clippy (`--all-targets -D warnings`) passes, and `cargo fmt` is clean.
- **JVM, Spark 4.1 (the default profile):**
- `CometFloatSemanticsSuite` and the `CometNativeReaderSuite` row-group pruning tests, including "row-group statistics pruning fires for a floating-point comparison": 415 passed.
- `CometSqlFileTestSuite` for `expressions/conditional/`, `expressions/math/`, `expressions/aggregate/`, `expressions/array/`, `windows/` and `join/`: 233 tests over 189 fixtures passed, including `float_comparisons.sql`, `nan_divisor.sql` and `arithmetic.sql`.
- **Not run locally:** the other Spark profiles.
### Benchmarks
These were measured in release mode on an M3 Max that other builds were also using. For "before", the same bench sources were built against #6447's head f2c2fc4e5, and the two builds' runs were interleaved.
Flat comparisons, per batch of 8,192 doubles, median of three runs:
| Comparison | DataFusion `BinaryExpr` | #6447 | This PR |
| --- | ---: | ---: | ---: |
| column `<` column | 7.34 µs | 11.28 µs | 2.73 µs |
| column `<` literal | 4.16 µs | 6.06 µs | 1.20 µs |
| column `=` column | 5.83 µs | 9.74 µs | 2.95 µs |
| column `=` literal | 3.37 µs | 5.29 µs | 1.19 µs |
| column `<=>` column | 5.99 µs | 9.94 µs | 2.99 µs |
| column `<=>` literal | 3.42 µs | 5.41 µs | 1.17 µs |
That's 3.3x to 5.1x faster than #6447. With `-0.0`, NaN or a sign-bit NaN in every tenth row, the numbers stay within a few percent. In #6447's build, the `normalized` engine matches `spark_comparison` to within 1%.
The DataFusion column comes from #6447's build. In this PR's build, DataFusion's `BinaryExpr` measured 40-60% slower on the same code: `normalize_float_zero` scans each column for `-0.0` with an `any()` loop that doesn't vectorize, and how fast that runs depended on the build. It's the same in both builds once the data holds a `-0.0`, because the scan then stops early.
Nested comparisons of 8,192 rows of `ARRAY<DOUBLE>`, where the null rows hold real lists:
| List width | Lists | Null rows | `<`: #6447 | `<`: this PR | `<=>`: #6447 | `<=>`: this PR |
| ---: | --- | --- | ---: | ---: | ---: | ---: |
| 1 | differ in the first element | none | 40.2 µs | 26.5 µs | 40.8 µs | 29.8 µs |
| 1 | differ in the first element | 1 in 16 | 36.5 µs | 28.9 µs | 36.4 µs | 33.3 µs |
| 1 | differ in the first element | 1 in 2 | 27.7 µs | 16.2 µs | 27.6 µs | 18.4 µs |
| 1 | equal | none | 37.5 µs | 36.5 µs | 35.5 µs | 33.5 µs |
| 1 | equal | 1 in 16 | 37.3 µs | 29.5 µs | 38.0 µs | 35.2 µs |
| 1 | equal | 1 in 2 | 27.8 µs | 16.5 µs | 28.5 µs | 19.5 µs |
| 16 | differ in the first element | none | 95.3 µs | 29.3 µs | 88.7 µs | 31.2 µs |
| 16 | differ in the first element | 1 in 16 | 88.9 µs | 29.7 µs | 89.3 µs | 34.1 µs |
| 16 | differ in the first element | 1 in 2 | 79.8 µs | 16.6 µs | 81.1 µs | 19.2 µs |
| 16 | equal | none | 223.2 µs | 211.3 µs | 223.6 µs | 213.8 µs |
| 16 | equal | 1 in 16 | 222.8 µs | 199.5 µs | 224.7 µs | 204.3 µs |
| 16 | equal | 1 in 2 | 150.9 µs | 107.5 µs | 150.2 µs | 109.0 µs |
| 1024 | differ in the first element | none | 3.69 ms | 73.6 µs | 3.72 ms | 82.6 µs |
| 1024 | differ in the first element | 1 in 16 | 3.76 ms | 70.5 µs | 3.75 ms | 80.5 µs |
| 1024 | differ in the first element | 1 in 2 | 3.80 ms | 56.6 µs | 3.70 ms | 63.0 µs |
| 1024 | equal | none | 13.19 ms | 11.87 ms | 13.12 ms | 11.90 ms |
| 1024 | equal | 1 in 16 | 12.49 ms | 11.27 ms | 12.45 ms | 11.29 ms |
| 1024 | equal | 1 in 2 | 8.96 ms | 6.67 ms | 9.07 ms | 6.58 ms |
Nested `=` already compared in place before this PR. Skipping null rows makes it 1.1x to 1.7x faster when there are nulls. Without nulls its code path does the same work as before, and it measured between 0.87x and 1.13x of #6447 across widths. A build of this branch that still had #6447's `=` code showed the same spread against #6447's build, so I read that as build-to-build noise rather than a change in the comparison.
My first version compared every row and tested the null buffers inside the comparator. Width-1 lists with sparse nulls were slower than #6447 that way, and so were dense nulls once the null rows held data. Walking the validity bitmap fixed both.
To reproduce:
```shell
cargo bench -p datafusion-comet-spark-expr --bench float_comparison
cargo bench -p datafusion-comet-spark-expr --bench nested_comparison -- 'nested_comparison/(lt|not_distinct|eq)/'
```
--
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] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691#discussion_r4213005670
##########
native/spark-expr/src/array_funcs/nested_comparison.rs:
##########
@@ -217,31 +311,176 @@ impl PhysicalExpr for NestedPredicate {
}
}
+/// A comparison of two Float32 or Float64 operands, or of two lists or structs with a float leaf,
+/// in Spark's SQL ordering: `-0.0` equals `0.0`, all NaNs are equal, and NaN sorts above every
+/// other value. It reads the operands as they are, without normalized copies of them.
+#[derive(Debug, Eq)]
+pub struct SparkComparison {
Review Comment:
Before this PR, a flat float comparison came out as `BinaryExpr(FloatNormalize(d) < FloatNormalize(e))`. `is_infallible` in `conditional_funcs/case_when.rs` accepts both of those nodes, so the comparison counted as infallible. It doesn't recognize `SparkComparison`, though. A `CASE WHEN` or `IF` that has a float comparison in a later `WHEN` or in a branch value is no longer infallible, so it falls back from #6350's eager evaluation to lazy evaluation.
I checked this with `is_infallible(spark_comparison(...))` for `d < 1.5`, `d = e` and `d <=> e`. It returns `true` on #6447's build and `false` on this branch. Then I timed this query over 8192 rows in release mode:
```sql
CASE WHEN i > 0 THEN 1 WHEN d < 0.5 THEN 2 WHEN d > 2.0 THEN 3 ELSE 4 END
```
It took about 22µs per batch before and about 75µs on this branch.
Could `is_infallible` accept a `SparkComparison`? Flat Float32/Float64 operands of the same type can't fail. The nested case only fails on a type mismatch, which planning already rules out. Could you also add a case to the `is_infallible` test in `case_when.rs` that builds the comparison through `spark_comparison` rather than by hand, so this can't silently regress again?
--
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] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691#discussion_r4220321902
##########
native/spark-expr/src/array_funcs/nested_comparison.rs:
##########
@@ -217,31 +311,176 @@ impl PhysicalExpr for NestedPredicate {
}
}
+/// A comparison of two Float32 or Float64 operands, or of two lists or structs with a float leaf,
+/// in Spark's SQL ordering: `-0.0` equals `0.0`, all NaNs are equal, and NaN sorts above every
+/// other value. It reads the operands as they are, without normalized copies of them.
+#[derive(Debug, Eq)]
+pub struct SparkComparison {
Review Comment:
Good catch, thanks. Fixed in 5b0705ce9f. `is_infallible` now asks the `SparkComparison`, which counts as infallible when its two operands have the same type. Every flat `FLOAT` or `DOUBLE` comparison passes that check. For nested operands the check matters: the nested comparators reject a dictionary-encoded leaf against a plain one, and the logical type check in `spark_comparison` lets that pair through. Nested `=` and `<>` are still `NestedPredicate`s, which `is_infallible` didn't accept before this PR either, so they stay lazy.
The `is_infallible` test now builds `d < 1.5`, `d = e` and `d <=> e` through `spark_comparison`, plus `<` and `<=>` on two `ARRAY<DOUBLE>` columns, and checks that an array compared with an array of dictionary-encoded doubles still counts as fallible. Leaving `SparkComparison` out of `is_infallible` fails the test, and so does counting every `SparkComparison` as infallible.
I also ran your query over 8,192 rows in a scratch test. `CaseWhenExpr` takes the eager path again and returns the same rows as `CaseExpr`. In release mode that's about 16µs per batch, against about 90µs for the lazy path.
--
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] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691
--
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] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691#issuecomment-6074274607
Thanks @viirya
--
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] perf: compare floats in Spark's order without normalizing copies of the operands [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6691:
URL: https://github.com/apache/datafusion-comet/pull/6691#issuecomment-6062253488
The contributor guide changes you asked for are in 5b0705ce9f too. The `spark_comparison` bullet now says it compares the operands as they are: `FLOAT` and `DOUBLE` with `compare_float_arrays` and `compare_float_array_scalar`, and arrays and structs with `spark_comparator`, or `spark_equality` for `=`, `<>`, `<=>` and `IS DISTINCT FROM`. The data filter bullet says that with `spark.comet.parquet.rowFilterPushdown.enabled=true` they compare in Spark's ordering like every other comparison, and the helper table has a row for the two kernels. I also replaced "unwrapped" in the `FloatOperands::Raw` sentence with "as a plain `BinaryExpr`", since neither path wraps the column now.
--
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