Skip to content

fix: correctly rounded decimal to double/float cast matching BigDecimal.doubleValue/floatValue - #5684

Open
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:fix/decimal-to-float-correct-rounding
Open

fix: correctly rounded decimal to double/float cast matching BigDecimal.doubleValue/floatValue#5684
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:fix/decimal-to-float-correct-rounding

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5670.

Rationale for this change

CAST(decimal AS DOUBLE/FLOAT) was delegated to arrow-cast, which computes (unscaled as f64) / 10^scale and rounds twice (three times for FLOAT, via the f64 intermediate). Spark's Decimal.toDouble/toFloat use BigDecimal.doubleValue()/floatValue(), which round the exact decimal once, so results differed in the last ulp whenever |unscaled| > 2^53 — for DECIMAL(38,18) that is essentially every value of magnitude >= 0.01 (12345.6789 became 12345.678899999999; CAST(16777217.0000000001 AS FLOAT) gave 16777216 instead of 16777218).

What changes are included in this PR?

  • New Decimal128 -> Float64/Float32 kernels in numeric.rs mirroring Java's algorithm: a single IEEE division/multiplication when the unscaled value and the power of ten are both exact in the target type (|unscaled| <= 2^53 and scale <= 22 for f64, <= 2^24 / <= 10 for f32), otherwise a correctly rounded parse of the exact decimal <unscaled>e<-scale> from a stack buffer straight into the target width (never via f64 for FLOAT). Negative scales and out-of-range floats (±Infinity) behave like BigDecimal.
  • cast.rs routes (Decimal128, Float32|Float64) to the kernels for LEGACY, ANSI and TRY (the conversion cannot fail); Float32/Float64 are removed from is_df_cast_from_decimal_spark_compatible.

How are these changes tested?

  • Rust unit tests: the issue's values, ties-to-even cases (2^53+1, 2^24+1), 38-digit extremes, i128::MIN/MAX, zero, negative scales, null preservation in every eval mode, and a 30k-value randomized check that the fast paths match the correctly rounded exact path; the tests carry assert_ne! guards showing the old (unscaled as f64)/10^scale formula fails on the issue's values. Reference values were cross-checked with jshell (BigDecimal.doubleValue()/floatValue()). cargo test -p datafusion-comet-spark-expr: 663 passed; cargo clippy --all-targets --workspace -- -D warnings clean.
  • CometNativeCastSuite: DECIMAL(38,18) values from the issue added to the Float/Double tests, new DECIMAL(38,10) -> Float/Double tests (including 16777217.0000000001), and DecimalType(38,18) re-enabled in the array and nested-array cast matrices that test: enable nested array cast coverage #4278 had excluded for this mismatch. Suite passes on Spark 4.1.3 (170/170, 8 pre-existing ignores).

…al.doubleValue/floatValue

CAST(decimal AS DOUBLE/FLOAT) was delegated to arrow-cast, which computes
`(unscaled as f64) / 10^scale` and therefore rounds twice (three times for
FLOAT, via the f64 intermediate). Spark's Decimal.toDouble/toFloat use
BigDecimal.doubleValue()/floatValue(), which round the exact decimal once, so
the results differed in the last ulp whenever |unscaled| > 2^53 — for
DECIMAL(38,18) that is essentially every value of magnitude >= 0.01
(12345.6789 became 12345.678899999999).

Add Decimal128 -> Float64/Float32 kernels that mirror Java's algorithm: a
single IEEE division/multiplication when both the unscaled value and the
power of ten are exact in the target type, otherwise a correctly rounded
parse of the exact decimal (`<unscaled>e<-scale>`) straight into the target
width. Route (Decimal128, Float32|Float64) to them in cast.rs for every eval
mode and drop Float32/Float64 from is_df_cast_from_decimal_spark_compatible.

Add Rust unit tests (issue values, ties, extremes, negative scales, nulls in
every eval mode, and a 30k-value randomized fast-path vs exact-path check),
extend CometNativeCastSuite with the issue's DECIMAL(38,18) and DECIMAL(38,10)
values, and re-enable DecimalType(38,18) in the array cast matrices that
PR apache#4278 had excluded for this mismatch.

Closes apache#5670

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

The prior Arrow path converts the coefficient to f64 before applying the decimal scale, and narrows through f64 for FLOAT. Those intermediate roundings can differ from Spark's decimal conversion. This change gives Decimal128-to-DOUBLE/FLOAT dedicated kernels: arithmetic when both operands are exact in the target width, otherwise direct parsing of the exact decimal value. The new dispatch also reaches decimal values inside recursive casts, and the JVM tests restore high-precision decimal coverage.

I compared the implementation with the maintained Spark 3.5 and 4.0 branches (5947fd6e74a1 and 03f28fc43180). Both interpreted and generated casts reach Decimal.toDouble / toFloat, which use BigDecimal. The separate target-width conversions avoid the FLOAT double-rounding problem. The arithmetic bounds are sufficient for one correctly rounded operation, and the parser preserves the coefficient and scale exactly. No correctness finding remained after checking signs, zero, ties, precision boundaries, nulls, empty/sliced arrays, and the scalar and recursive dispatch paths.

LEGACY, ANSI and TRY share these decimal-to-floating semantics; floating overflow produces infinity rather than a decimal overflow error. Negative scales are supported when spark.sql.legacy.allowNegativeScaleOfDecimal is enabled, so FLOAT infinity is reachable there. FLOAT subnormals are reachable at scale 38. DOUBLE overflow/subnormals are outside the Decimal128/i8-scale range. Other source/target fallback decisions are unchanged; removing FLOAT/DOUBLE from the Arrow-compatibility matrix accompanies the dedicated dispatch.

Validation and scope

Reviewed HEAD f977e8f4a2ec74bd7a598f02586d5974e8afadd5 against BASE 81d637b9bf40a5be6f4f0c65ad6f497b34746e69. An isolated harness containing the unchanged new kernels matched Java BigDecimal 26.0.1 and an independent integer-rational reference for 367,682 inputs at both widths, with zero bit mismatches; 128 Arrow array checks passed. Cases distinguish valid Spark decimals from extended i128/i8 stress inputs. This was component validation using Arrow 58.4.0 and its own lockfile, not a local full Comet or Spark build.

CI ran merge b561865f30998ca62872747195d7af25510e94de, whose parents are the exact base/head above. The changed files and native lockfile are identical to HEAD. The Rust job passed all four added tests (1,115 passed, 4 skipped overall). Successful Spark 3.5/JDK 17 and Spark 4.0/JDK 21 logs include the decimal-to-FLOAT/DOUBLE cases and array-cast suite. The review snapshot at 2026-09-04T17:24:25.103Z contains 65 successful and 9 skipped checks.

Performance

One P2 finding: handle scale zero before the exact-integer magnitude guard in both scalar helpers. Larger DECIMAL(p,0) coefficients currently incur formatting and parsing for each value even though a direct cast to the target float already performs the required single rounding. The inline comment identifies the branch and proposed change.

Matched Arrow-array benchmarks measured the following median nanoseconds per input slot, including output allocation:

Cast, no nulls Prior Arrow PR kernel Scale-zero direct-cast candidate
DECIMAL(18,0) → DOUBLE 1.53 33.49 1.53
DECIMAL(38,0) → DOUBLE 1.51 59.27 1.52
DECIMAL(12,0) → FLOAT 1.53 35.01 1.63

The candidate remained about 18× and 31× faster for the two DOUBLE datasets with approximately 20% nulls. Direct target casts matched the PR, Java and the rational reference on 162,083 scale-zero inputs, including midpoint cases. FLOAT must cast directly from i128 to f32; using the old f64 intermediate can reintroduce rounding differences.

These are component timings on Apple M5 Max, rustc 1.97.1 release and Arrow 58.4.0: 8,192-element arrays, nine alternating-order rounds, varied signed inputs, black-boxed inputs/outputs, and at least 25 ms calibration per strategy. They are not end-to-end Spark speedups. The candidate keeps nonzero-scale behavior unchanged, with similar control timings. Parsing on the general nonzero-scale path also costs substantially more than the prior arithmetic, but that arithmetic is not a correctness-preserving replacement.

Design

The dedicated kernels are a sensible boundary for Spark-specific rounding semantics. Keeping FLOAT separate from DOUBLE is necessary, and the exact-arithmetic fast path plus exact-decimal fallback is straightforward to audit. The scale-zero change is a small local improvement within this design; it requires no new dependency or conversion framework.

The fixed 48-byte buffer is sufficient: an i128 sign and 39 digits, exponent separator, and four exponent characters require at most 45 bytes. Widening the scale before negation handles i8::MIN. Valid ASCII decimal literals make the private parser's error expectations defensible. Negative scales work even though one comment describes only the default nonnegative-scale case.

Abstraction & complexity

The two small Arrow wrappers share one private formatter/parser and reuse Arrow's null-preserving unary operation. This keeps the Spark-specific behavior close to the conversion dispatch without introducing wider state or indirection. The generic parser has only the two intended float instantiations, which earns the small amount of abstraction. No additional design or abstraction finding was identified.

pub(crate) fn decimal128_to_f64(unscaled: i128, scale: i8) -> f64 {
// Fast path (also Java's): when the unscaled value and the power of ten are both exact
// doubles, a single IEEE division or multiplication is correctly rounded.
if unscaled.unsigned_abs() <= F64_EXACT_INT_LIMIT {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance

[P2] Handle scale zero before the exact-integer guard

Please handle scale == 0 before this magnitude guard in both helpers. Above 2^53 (2^24 for FLOAT), DECIMAL(p,0) currently formats and parses every value, although a direct i128 as f64 / i128 as f32 already performs the required single, correctly rounded conversion. In matched 8,192-element Arrow 58.4.0 array benchmarks (M5 Max, rustc 1.97.1 release), DECIMAL(18,0) → DOUBLE rose from 1.53 ns/input slot in Arrow to 33.49 ns here; handling scale zero first took 1.53 ns. For DECIMAL(38,0), this was 59.27 ns versus 1.52 ns, with the gain persisting with nulls. Direct target casts matched HEAD, Java BigDecimal and an exact rational reference on 162,083 scale-zero inputs, including ties. This avoids the regression while retaining the exact nonzero-scale path. FLOAT should cast directly to f32, without an f64 intermediate. These are component timings, not end-to-end Spark speedups. Please add scale-zero FLOAT/DOUBLE microbenchmark cases, including nulls, to keep this fast path covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decimal to double/float cast is not correctly rounded: last-digit-wrong results for ordinary DECIMAL(38,18) values

2 participants