peterxcli opened a new pull request, #5683:
URL: https://github.com/apache/datafusion-comet/pull/5683
## Which issue does this PR close?
Closes #5673.
## Rationale for this change
In ANSI mode the native cast from `FLOAT`/`DOUBLE` to `INT`/`BIGINT`
detected overflow as "the saturating conversion of `|value|` landed on `MAX`"
(`value.abs() as $rust_dest_type == $max_dest_val`). That rejects every double
in `[2147483647.0, 2147483648.0)` and `(-2147483649.0, -2147483647.0]` for
`INT`, including the exactly representable `Int.MaxValue`/`Int.MinValue`, and
rejects `±2^63` for `BIGINT` even though `Long.MaxValue.toDouble == 2^63` and
Spark accepts it. Spark's
`FloatExactNumeric`/`DoubleExactNumeric.toInt`/`toLong` accept any `x` with
`Math.floor(x) <= MaxValue && Math.ceil(x) >= MinValue` (in double precision)
and return `x.toInt`/`x.toLong`, which truncate towards zero and saturate. So
`CAST(2147483647.0D AS INT)`, `CAST(-2147483648.0D AS INT)`,
`CAST(2147483647.5D AS INT)` and `CAST(9223372036854775808.0D AS BIGINT)`
succeed in Spark but raised `CAST_OVERFLOW` in Comet. The cast is enabled by
default (`Compatible` in `CometCast`), so every ANSI user was affec
ted.
## What changes are included in this PR?
- `numeric.rs`: new `spark_float_fits_integral` helper implementing Spark's
bound check (source widened to `f64` as the JVM does for
`Math.floor`/`Math.ceil`, bounds converted to `f64`), after which `as`
truncates and saturates like the JVM's `d2i`/`d2l`, so `±2^63 → BIGINT`
saturates to `Long.MaxValue`/`Long.MinValue` exactly like Spark. NaN fails both
comparisons, the infinities fail one.
- `cast_float_to_int32_up` (INT/BIGINT) uses the helper; the now-unused
`$max_dest_val` macro argument is removed from the macro and its four call
sites.
- `cast_float_to_int16_down` (TINYINT/SMALLINT) uses the helper for its
first step. Spark's ANSI `castToByte`/`castToShort` run `exactNumeric.toInt`
and then require the truncated `Int` to round-trip through `toByte`/`toShort`;
the macro already implemented the second step, and it rejected every value the
wrong first step flagged, so the observable behaviour of these casts does not
change.
- LEGACY mode is untouched (it already saturated like Spark's non-ANSI
`toInt`/`toLong`); TRY mode does not go through these macros (`cast.rs` routes
it to Arrow's cast).
- `CometNativeCastSuite`: eight `cast {Float,Double}Type to
{Integer,Long,Short,Byte}Type - ANSI boundary values` tests comparing Spark and
Comet on the exactly representable bounds, fractional values just inside them,
`±2^63` for `BIGINT`, and out-of-range values that must overflow (cast one at a
time so each is checked in both engines, in LEGACY, ANSI and `try_cast` mode).
Two related discrepancies were noticed and deliberately left out of this PR:
1. `try_cast(9223372036854775808.0D AS BIGINT)` (and the `FLOAT` equivalent)
returns `NULL` natively because Arrow's float→int cast requires `x < 2^63`,
whereas Spark returns `Long.MaxValue`. The new tests skip the `try_cast`
comparison for that single value (TODO).
2. The `CAST_OVERFLOW` message renders the offending value with Rust's
`{:e}` formatting, which only matches Java's `Double.toString`/`Float.toString`
for large values with several significant digits (`CAST(128.0D AS TINYINT)`
reports `1.28E2D` where Spark reports `128.0D`; NaN/Infinity become
`NaND`/`infD`). The Scala tests use overflowing values from the range where
both agree; the exact bounds of the narrower targets are covered by the Rust
unit tests.
## How are these changes tested?
- New Rust unit tests in `numeric.rs`
(`test_cast_double_to_int_ansi_boundaries`,
`test_cast_double_to_long_ansi_boundaries`,
`test_cast_float_to_int_ansi_boundaries`,
`test_cast_float_to_long_ansi_boundaries`,
`test_cast_float_to_short_and_byte_ansi_boundaries`,
`test_cast_float_to_integral_legacy_saturates`). The first four fail on `main`
with the reported errors (e.g. `CastOverFlow { value: "2.147483647E9D",
from_type: "DOUBLE", to_type: "INT" }`) and pass with the fix; expected values
were cross-checked against the JVM on JDK 11, 17 and 21.
- New `CometNativeCastSuite` tests described above. With the default Spark
4.1 profile on JDK 17, `./mvnw test -Dtest=none
-Dsuites="org.apache.comet.CometNativeCastSuite"` passes: `Tests: succeeded
176, failed 0, canceled 0, ignored 8` (ignores pre-existing).
- `cargo fmt`, `cargo clippy --all-targets --workspace -- -D warnings` and
`spotless:apply` are 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]
