andygrove opened a new pull request, #5600:
URL: https://github.com/apache/datafusion-comet/pull/5600
## Which issue does this PR close?
Closes #5576.
## Rationale for this change
`round` on a float or double fell the whole projection back to Spark:
```scala
case _: FloatType | DoubleType =>
Unsupported(Some("Comet does not support Spark's BigDecimal rounding"))
```
The reasoning behind that is sound — Spark rounds floating point through a
`BigDecimal` built from `java.lang.Double.toString()`, and no native kernel
reproduces that. But "we can't do this natively" is precisely what the JVM
codegen dispatcher is for: running Spark's own `RoundBase.doGenCode` inside the
Comet pipeline gives that behavior byte for byte. `bround` — the sibling
function with the *same* `doGenCode` — already dispatches, so the two behaved
completely differently for no principled reason.
`round(double_col, n)` is everyday analytics SQL. TPC-DS q78 is a concrete
example: it goes from **72/74 eligible operators (97%) to 74/74 (100%)**, and
`10 native, 0 codegen dispatch` to `12 native, 1 codegen dispatch` — the
projection that used to fall back was also carrying two other expressions that
could have run natively.
## What changes are included in this PR?
- `CometRound` mixes in `CodegenDispatchFallback`, adds
`getUnsupportedReasons()`, and reuses those reason strings in
`getSupportLevel`. Integral and non-negative-scale decimal inputs are
unaffected and still take the native path.
- The issue suggested leaving the negative-scale decimal arm alone. It turns
out to dispatch cleanly — `canHandle` admits it and the result matches Spark —
so both `Unsupported` arms now stay in the Comet pipeline rather than falling
back. There is a test pinning that.
- Removes the `Float32`/`Float64` arms of the native `spark_round`. They
were unreachable: `CometRound.convert` is the only thing that emits the `round`
scalar function and it never emits it for those types. Their unit tests go with
them.
- `expressions.md`: `round` is now `Hybrid` rather than `Native`, and the
note says where float/double actually run.
- Regenerated the two q78 plan-stability goldens.
## How are these changes tested?
`round.sql` previously marked every query `expect_fallback(BigDecimal
rounding)`. Those are now plain `query` records, i.e.
`checkSparkAnswerAndOperator`, which fails if anything falls back. The fixture
is also extended with a float column, NaN / ±Infinity, null scale, the one-arg
form, and the two pathological doubles named in the source comment —
`-5.81855622136895E8` and `6.1317116247283497E18` — whose shortest decimal
representation rounds differently than the exact binary value. Those are the
cases a native implementation would get wrong, so they are the ones worth
pinning.
New `CometExpressionSuite` test for negative-scale decimals under
`spark.sql.legacy.allowNegativeScaleOfDecimal=true` (built via the DataFrame
API, since the type cannot be spelled in SQL syntax).
Run locally:
- `CometSqlFileTestSuite` round fixture and the three `CometExpressionSuite`
round tests, on Spark 4.1 and 3.4.
- Both plan-stability suites in check mode on Spark 3.4 and 4.2; goldens
regenerated across 3.4 / 3.5 / 4.0 / 4.1 / 4.2, and only the two base q78 files
changed — the version-specific directories pruned as duplicates, so every
supported version produces the same plan.
- `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check`, and the
`spark-expr` round unit tests; `spotless:check` and `scalastyle:check`.
--
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]
