fix: match Spark's ANSI bound check for float/double to integral casts - #5683
Conversation
In ANSI mode the native cast from FLOAT/DOUBLE to INT/BIGINT detected overflow as "the saturating conversion of |value| landed on MAX", which rejects every value 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. Replace the check in cast_float_to_int32_up and cast_float_to_int16_down with Spark's FloatExactNumeric/DoubleExactNumeric semantics: `Math.floor(x) <= MaxValue && Math.ceil(x) >= MinValue`, evaluated in double precision, followed by a truncating, saturating conversion (Rust's `as` behaves like the JVM's d2i/d2l). NaN and the infinities still overflow. Casts to TINYINT/SMALLINT keep Spark's two-step behaviour (INT range check, then the truncated INT must fit the narrower type); their observable behaviour is unchanged. LEGACY mode is untouched and TRY mode does not go through these macros. Closes apache#5673 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This replaces the absolute-value/saturating-cast overflow heuristic with Spark's ANSI bound check for FLOAT/DOUBLE to integral casts. I compared the implementation with Spark 3.5 and 4.0 FloatExactNumeric and DoubleExactNumeric, including interpreted/generated cast routing and the separate Legacy and Try paths. Widening the source to f64 before floor/ceil matches Spark's comparison semantics. It admits the exactly representable INT boundaries and fractions whose truncation remains in range, while rejecting NaN and infinities.
The BIGINT boundary deserves special care: converting Long.MaxValue to double produces 2^63, so Spark permits that floating-point value and the subsequent JVM conversion saturates to Long.MaxValue. The Rust check followed by as reproduces that result. BYTE and SHORT still pass through the INT check and then require the narrowing conversion to round-trip. Nulls remain null. Legacy behavior is unchanged, and Try still follows the existing Arrow route. The observed Try result at positive 2^63 and existing error-string differences are pre-existing, not regressions introduced by this patch. I found no new P1/P2 issue.
Validation and CI
A focused harness compiled the exact old/new conversion macros, eight floating-point dispatch arms and overflow helper. It checked 22,180 inputs and 709,760 old/new component cases per compiler target against an exact integer-ratio oracle and JVM saturation/narrowing rules. Coverage included adjacent boundary values, signed zero, subnormals, NaN and infinity behind null masks, sliced arrays, empty arrays and all-null arrays. The harness used small local mode/error shells, so these results do not establish full JVM exception-message parity. I did not run a local full Comet crate, JNI path or Spark suite.
The final CI refresh showed 64 successful checks, nine skipped and one failed. The failed Spark 3.5/JDK 17 Java lint job stopped while Maven resolved commons-parent:pom:52 through commons-codec:1.15, with HTTP 502, before scalafix ran. That is a dependency-fetch failure, not evidence of a source lint violation. I did not rerun CI and am not treating this as an all-green result.
Performance
I measured the exact component paths with Rust 1.97.1 and Arrow 58.4 on an AMD EPYC-Milan host, with a fixed CPU and seven alternating samples after warm-up. For the x86-64-v3 target used by the repository's Linux amd64 release build, 65,536-row ANSI cases measured about 0.89–1.04 times the previous time. The sampled smaller arrays were also around parity. This did not show a material regression for that measured production target.
Compiler target matters here. A generic Rust target was about 1.40–1.79 times slower for the large ANSI cases. I checked the Makefile rather than presenting those generic-target numbers as the release configuration. The results do not cover ARM, macOS or full Spark query throughput. The helper adds no allocation, and Legacy/Try retain their existing paths.
Design
The new predicate directly expresses the rule used by Spark instead of trying to infer overflow from a value that has already saturated during conversion. That makes the boundary behavior easier to reason about, especially for BIGINT where the floating-point representation of the maximum is itself outside the mathematical integer range.
Keeping the range check separate from the conversion is appropriate. The comparison is performed in the same widened domain as Spark, then the existing cast supplies the matching truncation/saturation behavior. The BYTE/SHORT round-trip check remains explicit, so the change does not accidentally replace Spark's two-stage narrowing semantics with a direct cast.
Abstraction & complexity
The small spark_float_fits_integral helper centralizes one shared Spark rule across the existing conversion macros. Its Into<f64> bound captures the required widening for both input widths without adding a new trait hierarchy or public API. Removing the separate maximum-value macro argument also reduces duplicated type-bound information.
The patch stays within the established dispatch and error paths. I did not find an unnecessary new abstraction or a broader refactor required for these semantics. The remaining macro structure is existing implementation machinery, and the new helper makes its correctness condition more explicit.
|
Merged, thanks @peterxcli ! |
Which issue does this PR close?
Closes #5673.
Rationale for this change
In ANSI mode the native cast from
FLOAT/DOUBLEtoINT/BIGINTdetected overflow as "the saturating conversion of|value|landed onMAX" (value.abs() as $rust_dest_type == $max_dest_val). That rejects every double in[2147483647.0, 2147483648.0)and(-2147483649.0, -2147483647.0]forINT, including the exactly representableInt.MaxValue/Int.MinValue, and rejects±2^63forBIGINTeven thoughLong.MaxValue.toDouble == 2^63and Spark accepts it. Spark'sFloatExactNumeric/DoubleExactNumeric.toInt/toLongaccept anyxwithMath.floor(x) <= MaxValue && Math.ceil(x) >= MinValue(in double precision) and returnx.toInt/x.toLong, which truncate towards zero and saturate. SoCAST(2147483647.0D AS INT),CAST(-2147483648.0D AS INT),CAST(2147483647.5D AS INT)andCAST(9223372036854775808.0D AS BIGINT)succeed in Spark but raisedCAST_OVERFLOWin Comet. The cast is enabled by default (CompatibleinCometCast), so every ANSI user was affected.What changes are included in this PR?
numeric.rs: newspark_float_fits_integralhelper implementing Spark's bound check (source widened tof64as the JVM does forMath.floor/Math.ceil, bounds converted tof64), after whichastruncates and saturates like the JVM'sd2i/d2l, so±2^63 → BIGINTsaturates toLong.MaxValue/Long.MinValueexactly 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_valmacro 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 ANSIcastToByte/castToShortrunexactNumeric.toIntand then require the truncatedIntto round-trip throughtoByte/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.toInt/toLong); TRY mode does not go through these macros (cast.rsroutes it to Arrow's cast).CometNativeCastSuite: eightcast {Float,Double}Type to {Integer,Long,Short,Byte}Type - ANSI boundary valuestests comparing Spark and Comet on the exactly representable bounds, fractional values just inside them,±2^63forBIGINT, and out-of-range values that must overflow (cast one at a time so each is checked in both engines, in LEGACY, ANSI andtry_castmode).Two related discrepancies were noticed and deliberately left out of this PR:
try_cast(9223372036854775808.0D AS BIGINT)(and theFLOATequivalent) returnsNULLnatively because Arrow's float→int cast requiresx < 2^63, whereas Spark returnsLong.MaxValue. The new tests skip thetry_castcomparison for that single value (TODO).CAST_OVERFLOWmessage renders the offending value with Rust's{:e}formatting, which only matches Java'sDouble.toString/Float.toStringfor large values with several significant digits (CAST(128.0D AS TINYINT)reports1.28E2Dwhere Spark reports128.0D; NaN/Infinity becomeNaND/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?
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 onmainwith 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.CometNativeCastSuitetests 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 warningsandspotless:applyare clean.