andygrove opened a new pull request, #5136: URL: https://github.com/apache/datafusion-comet/pull/5136
## Which issue does this PR close? Closes #1371. ## Rationale for this change Spark casts float/double to decimal by rounding the decimal string form of the value: `Decimal(double)` goes through `BigDecimal(Double.toString(d))`, then `changePrecision` applies `setScale(scale, HALF_UP)`. Comet instead computed `(f * 10^scale).round()` on the binary value. The two disagree whenever the shortest decimal representation and the true binary value fall on opposite sides of a rounding tie. For example, `0.5153125` is `0.51531249999999995...` in binary, so at scale 6 Spark rounds up to `0.515313` while Comet rounded down to `0.515312`. This made the cast `Incompatible` (opt-in only) and was a correctness issue for deterministic data. There was a second divergence: for NaN and infinity, Spark returns null even in ANSI mode (the `NumberFormatException` from `Decimal(double)` is caught and swallowed before the ANSI overflow check), while Comet raised `NUMERIC_VALUE_OUT_OF_RANGE`. ## What changes are included in this PR? - `cast_floating_point_to_decimal128` now produces Spark-exact results. The exact path formats the value with `ryu` (shortest round-trip decimal form, the same digits as `Double.toString`) and rescales with pure `i128` integer math using HALF_UP rounding. No JDK code is used; `ryu` is Apache-2.0/BSL-1.0 dual licensed and was already in the dependency tree. - A guarded fast path keeps the previous vectorized multiply-and-round for values that are provably far enough from a `.5` rounding boundary that binary and string rounding must agree, so the exact path only runs for tie-adjacent values, which are vanishingly rare in real data. The `cast_float_to_decimal` benchmark shows ~1.8x the previous per-row cost (~3.5ns/row, ~290M values/sec) instead of ~20x for the unguarded exact conversion. - Floats are widened to double before conversion, matching Spark's `x.toDouble`: `1.005f` casts to `1.00` at scale 2 (its widened value is `1.0049999952316284`), not `1.01`. - NaN and infinity now produce null in all eval modes, including ANSI, matching Spark. - `CometCast` reports float/double to decimal as `Compatible`, and the expression audit doc is updated. - The two ignored `CometCastSuite` tests are re-enabled and their `allow incompat` variants removed; new rounding edge case tests cover tie values on both sides of zero, round-up-into-overflow (`99.995` as `decimal(4,2)`), and the float widening behavior. The `avg decimal` aggregate test now asserts native aggregation. The MapType Incompatible-propagation test now uses the remaining incompatible cast (negative-scale decimal to string). One caveat: `ryu` matches `Double.toString` exactly on JDK 19+. Older JDKs occasionally emit one extra digit beyond the shortest form, so in the rare case where a value's shortest form ends exactly on a tie at the target scale, Spark on an older JDK could round differently. This is a case where Spark's own result is JDK-dependent. ## How are these changes tested? Tests were written first against Spark's documented semantics and observed failing on the previous implementation (this work followed the test-driven-development skill). New Rust unit tests cover the tie values, HALF_UP away-from-zero rounding for negatives, exponent-notation forms, subnormals, float widening, round-up-into-overflow in both LEGACY and ANSI, and ANSI NaN/infinity nulls. A differential test runs 240k values (random bit patterns plus decimal-shaped values) at six precision/scale combinations and asserts the guarded fast path always agrees with the exact path. On the JVM side, the previously ignored `CometCastSuite` float/double to decimal tests are re-enabled, new edge case tests added, and `CometAggregateSuite` "avg decimal" now checks native aggregation; all pass locally along with the surrounding decimal cast tests, and `cargo clippy` is clean. -- 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]
