You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/29 16:20:13 UTC
[PR] fix: make cast from float/double to decimal compatible with Spark [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#discussion_r3691572509
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
+ } else {
+ match pow10_i128(-shift as u32) {
+ // The mantissa has at most 17 significant digits, so dividing by a power of
+ // ten too large for i128 always rounds to zero
+ None => 0,
+ // Divide with HALF_UP rounding (away from zero on a tie, matching BigDecimal)
+ Some(div) => {
+ let quotient = mantissa / div;
+ let remainder = mantissa % div;
+ if remainder.abs() >= div / 2 {
+ quotient + mantissa.signum()
+ } else {
+ quotient
+ }
+ }
+ }
+ };
+
+ is_validate_decimal_precision(unscaled, precision).then_some(unscaled)
+}
+
+/// Parse ryu's `[-]digits[.digits][e[-]digits]` output into an integer mantissa and a
+/// base-10 exponent such that the value equals `mantissa * 10^exp10`. The mantissa of a
+/// shortest-form double has at most 17 significant digits so it cannot overflow i128.
+fn parse_decimal_notation(s: &str) -> (i128, i32) {
+ let (digits, exp10) = match s.split_once('e') {
+ Some((digits, exp)) => (digits, exp.parse::<i32>().expect("exponent from ryu")),
+ None => (s, 0),
+ };
+ let mut mantissa: i128 = 0;
+ let mut frac_digits = 0;
+ let mut in_fraction = false;
+ for b in digits.bytes() {
+ match b {
+ b'-' => {}
+ b'.' => in_fraction = true,
+ _ => {
+ mantissa = mantissa * 10 + (b - b'0') as i128;
+ if in_fraction {
+ frac_digits += 1;
+ }
+ }
+ }
+ }
+ if digits.starts_with('-') {
+ mantissa = -mantissa;
+ }
+ (mantissa, exp10 - frac_digits)
+}
Review Comment:
`parse_decimal_notation` hand-rolls a byte-by-byte digit accumulator (`mantissa = mantissa * 10 + (b - b'0') as i128`) to turn ryu's `[-]digits[.digits][e[-]digits]` output into a mantissa and exponent. `string.rs` already has `digits_to_i128` (`string.rs:493-503`) that does exactly this accumulation with overflow checks, and `parse_decimal_str` (`string.rs:662-745`) already shows the pattern of splitting a decimal string on `.` into an integral and fractional part and combining them via `integral * 10^frac_digits + fractional` (`string.rs:735-738`). Rewrite `parse_decimal_notation` to strip the sign, split on `.`, and call `digits_to_i128` on each part the same way `parse_decimal_str` does, instead of reimplementing the digit scan. The one real difference - ryu's output is always well-formed so this function does not need `parse_decimal_str`'s validation/error path - is still true after the rewrite, since `digits_to_i128` only returns `None` on overflow, which cannot happen for a
17-significant-digit mantissa.
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -436,8 +435,7 @@ object CometCast
DataTypes.IntegerType | DataTypes.LongType | DataTypes.TimestampType =>
Compatible()
case _: DecimalType =>
- // https://github.com/apache/datafusion-comet/issues/1371
- Incompatible(Some("There can be rounding differences"))
+ Compatible()
Review Comment:
Same issue as the `Float -> Decimal` arm above: `case _: DecimalType => Compatible()` for `Double -> Decimal` should carry the same JDK < 19 tie-rounding caveat as a `Compatible(Some(...))` note.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
Review Comment:
`mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?` converts `shift: i32` to `u32` with a fallible `try_into().ok()?`, but this line only runs inside the `if shift >= 0` branch two lines above, so the conversion can never actually fail here - `shift` is already known to be non-negative and an `i32 -> u32` conversion of a non-negative value always succeeds. The sibling `else` branch immediately below uses a plain `-shift as u32`, and every other `pow10_i128` call in this crate (`string.rs:596`, `string.rs:605`, `string.rs:735`) uses a plain `as u32` cast. Change this to `pow10_i128(shift as u32)?` so it matches the sibling branch and the rest of the file instead of disguising a guaranteed-safe cast as fallible.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
+ } else {
+ match pow10_i128(-shift as u32) {
+ // The mantissa has at most 17 significant digits, so dividing by a power of
+ // ten too large for i128 always rounds to zero
+ None => 0,
+ // Divide with HALF_UP rounding (away from zero on a tie, matching BigDecimal)
+ Some(div) => {
+ let quotient = mantissa / div;
+ let remainder = mantissa % div;
+ if remainder.abs() >= div / 2 {
+ quotient + mantissa.signum()
+ } else {
+ quotient
+ }
+ }
Review Comment:
The `Some(div) => { ... }` arm computes `quotient = mantissa / div`, `remainder = mantissa % div`, and rounds away from zero when `remainder.abs() >= div / 2`. This is the third copy of the same HALF_UP-by-power-of-ten integer division in this crate: `parse_decimal_str` in `string.rs:605-624` does the identical quotient/remainder/half-divisor rounding for `i128` (just spelled with an explicit `if mantissa >= 0 { quotient + 1 } else { quotient - 1 }` instead of `mantissa.signum()`), and `div_round_half_up` in `native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:121-144` does the same thing again for `i256`. Factor this into one `pub(crate) fn div_round_half_up_i128(numerator: i128, divisor: i128) -> i128` (next to `pow10_i128` in `string.rs`, mirroring the `pow10_i128`/`digits_to_i128` visibility change already made in this diff) and call it from both `float_to_decimal128` and `parse_decimal_str`, instead of adding a third inline copy of the same rounding arithmetic.
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -425,8 +425,7 @@ object CometCast
DataTypes.IntegerType | DataTypes.LongType | DataTypes.TimestampType =>
Compatible()
case _: DecimalType =>
- // https://github.com/apache/datafusion-comet/issues/1371
- Incompatible(Some("There can be rounding differences"))
+ Compatible()
Review Comment:
`case _: DecimalType => Compatible()` for `Float -> Decimal` drops the caveat this PR's own description calls out: ryu matches `Double.toString` exactly only on JDK 19+, and on older JDKs `Double.toString` can emit one extra digit beyond the shortest round-trip form, so a value whose shortest form lands exactly on a rounding tie at the target scale can round differently than Spark on JDK < 19. `Compatible` already carries an optional note used elsewhere in this same file for exactly this kind of caveat (e.g. `Compatible(Some("Only supports years between 262143 BC and 262142 AD"))` for date-to-string a few lines up). Add a `Compatible(Some(...))` note here describing the JDK < 19 tie-rounding caveat instead of silently upgrading to a bare `Compatible()`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#issuecomment-5121356884
I need to run benchmarks to confirm that this is faster than the codegen dispatch approach which is being added in a separate PR
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#discussion_r3692058292
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
+ } else {
+ match pow10_i128(-shift as u32) {
+ // The mantissa has at most 17 significant digits, so dividing by a power of
+ // ten too large for i128 always rounds to zero
+ None => 0,
+ // Divide with HALF_UP rounding (away from zero on a tie, matching BigDecimal)
+ Some(div) => {
+ let quotient = mantissa / div;
+ let remainder = mantissa % div;
+ if remainder.abs() >= div / 2 {
+ quotient + mantissa.signum()
+ } else {
+ quotient
+ }
+ }
Review Comment:
Factored out in 069b1e0c1 as `div_round_half_up_i128` next to `pow10_i128` in `string.rs`, called from both `float_to_decimal128` and `parse_string_to_decimal`.
Two notes while collapsing them:
- The string path's `checked_div` / "check if divisor is 0" guard is gone. The divisor there is always `pow10_i128(abs_scale_adjustment)` with `abs_scale_adjustment >= 1`, so it can never be zero and that early-return was dead.
- The helper documents `divisor` as `10^n` for `n >= 1` and `debug_assert!`s it, because `divisor / 2` is only a correct half-way test for an even divisor: with `divisor == 1` the remainder is always 0 and `0 >= 0` would round every value away from zero. Both call sites guarantee `n >= 1`.
I left the `i256` `div_round_half_up` in `wide_decimal_binary_expr.rs` alone, since it is a different integer width — happy to unify that too if you would like it in this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#discussion_r3692059678
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
+ } else {
+ match pow10_i128(-shift as u32) {
+ // The mantissa has at most 17 significant digits, so dividing by a power of
+ // ten too large for i128 always rounds to zero
+ None => 0,
+ // Divide with HALF_UP rounding (away from zero on a tie, matching BigDecimal)
+ Some(div) => {
+ let quotient = mantissa / div;
+ let remainder = mantissa % div;
+ if remainder.abs() >= div / 2 {
+ quotient + mantissa.signum()
+ } else {
+ quotient
+ }
+ }
+ }
+ };
+
+ is_validate_decimal_precision(unscaled, precision).then_some(unscaled)
+}
+
+/// Parse ryu's `[-]digits[.digits][e[-]digits]` output into an integer mantissa and a
+/// base-10 exponent such that the value equals `mantissa * 10^exp10`. The mantissa of a
+/// shortest-form double has at most 17 significant digits so it cannot overflow i128.
+fn parse_decimal_notation(s: &str) -> (i128, i32) {
+ let (digits, exp10) = match s.split_once('e') {
+ Some((digits, exp)) => (digits, exp.parse::<i32>().expect("exponent from ryu")),
+ None => (s, 0),
+ };
+ let mut mantissa: i128 = 0;
+ let mut frac_digits = 0;
+ let mut in_fraction = false;
+ for b in digits.bytes() {
+ match b {
+ b'-' => {}
+ b'.' => in_fraction = true,
+ _ => {
+ mantissa = mantissa * 10 + (b - b'0') as i128;
+ if in_fraction {
+ frac_digits += 1;
+ }
+ }
+ }
+ }
+ if digits.starts_with('-') {
+ mantissa = -mantissa;
+ }
+ (mantissa, exp10 - frac_digits)
+}
Review Comment:
Rewritten in 069b1e0c1: `parse_decimal_notation` now strips the sign, splits on `.`, and calls `digits_to_i128` on each part, combining them via `integral * 10^frac_digits + fractional` the same way `parse_string_to_decimal` does. `digits_to_i128` is now `pub(crate)`.
One deliberate difference from your sketch: it returns `Option<(i128, i32)>` and `float_to_decimal128` propagates with `?`. `digits_to_i128` and the `10^frac_digits` combination are both fallible in the type system even though they cannot fail for ryu output, and propagating is better than the previous unchecked `mantissa * 10 + digit` (which would have wrapped) or an `expect` that panics inside a cast kernel. The doc comment states the invariant that makes the `None` arm unreachable.
Also added `test_parse_decimal_notation_round_trips_ryu_output`, which asserts the mantissa/exponent round-trips back to the original `f64` for every shape ryu emits (plain integral, plain fractional, leading-zero fractional with up to 21 fractional digits, and both exponent forms) plus `f64::MAX`, `f64::MIN`, `f64::MIN_POSITIVE`, and the smallest subnormal. That is the direct guard on the rewrite — the existing differential test only compares the two paths against each other, so it would not have caught a parser change that moved both.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#discussion_r3692057216
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -939,6 +961,79 @@ where
Ok(Arc::new(result.with_precision_and_scale(precision, scale)?))
}
+/// Convert a double to a decimal unscaled value with Spark semantics.
+///
+/// Spark converts through `BigDecimal(Double.toString(d)).setScale(scale, HALF_UP)`: it
+/// rounds the shortest decimal string form of the value, not its exact binary expansion.
+/// The two disagree for values like 0.5153125 whose binary value (0.51531249999...) sits
+/// just below the rounding tie that the string form lands on, so a plain
+/// `(f * 10^scale).round()` produces results that differ from Spark.
+///
+/// Returns `None` for NaN / infinity and for results that do not fit `precision`.
+fn float_to_decimal128(f: f64, precision: u8, scale: i8) -> Option<i128> {
+ if !f.is_finite() {
+ return None;
+ }
+
+ // Shortest round-trip decimal form, same digits as Java's Double.toString
+ let mut buf = ryu::Buffer::new();
+ let (mantissa, exp10) = parse_decimal_notation(buf.format_finite(f));
+
+ // value = mantissa * 10^exp10, so unscaled = round(mantissa * 10^(exp10 + scale))
+ let shift = exp10 + scale as i32;
+ let unscaled = if shift >= 0 {
+ // Overflowing i128 here means the result cannot fit any decimal precision
+ mantissa.checked_mul(pow10_i128(shift.try_into().ok()?)?)?
Review Comment:
Fixed in 069b1e0c1 — now `pow10_i128(shift as u32)?`, matching the sibling `else` branch and the other callers.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#discussion_r3692062238
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -425,8 +425,7 @@ object CometCast
DataTypes.IntegerType | DataTypes.LongType | DataTypes.TimestampType =>
Compatible()
case _: DecimalType =>
- // https://github.com/apache/datafusion-comet/issues/1371
- Incompatible(Some("There can be rounding differences"))
+ Compatible()
Review Comment:
Good catch — added in 069b1e0c1. Both the float and double `DecimalType` arms now return `Compatible(Some(floatToDecimalJdkNote))`:
> Rounding matches Spark on JDK 19 and later. On older JDKs, Spark's own result may differ for values whose shortest decimal form falls exactly on a rounding tie
The constant carries a longer comment explaining the `Double.toString` / JDK-4511638 background. The note flows into the generated `cast.md` matrix automatically via `writeCastMatrixForMode`, so nothing checked in needed regenerating. I also updated the cast audit doc, which had the same gap — it claimed a bare `Compatible` without the caveat.
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -436,8 +435,7 @@ object CometCast
DataTypes.IntegerType | DataTypes.LongType | DataTypes.TimestampType =>
Compatible()
case _: DecimalType =>
- // https://github.com/apache/datafusion-comet/issues/1371
- Incompatible(Some("There can be rounding differences"))
+ Compatible()
Review Comment:
Same fix applied here in 069b1e0c1 — the double arm shares the same `floatToDecimalJdkNote` constant.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make native cast from float/double to decimal compatible with Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5136:
URL: https://github.com/apache/datafusion-comet/pull/5136#issuecomment-5145724074
@mbutrovich thanks for the review — all five comments are addressed in 069b1e0c1, with replies inline on each thread. Summary:
- `float_to_decimal128` uses a plain `shift as u32` cast instead of the fallible `try_into().ok()?`.
- The HALF_UP-by-power-of-ten division is now a single `div_round_half_up_i128` next to `pow10_i128` in `string.rs`, called from both `float_to_decimal128` and `parse_string_to_decimal`. Collapsing them also removed a dead `checked_div`/zero-divisor guard in the string path. I left the `i256` copy in `wide_decimal_binary_expr.rs` alone as a different integer width — say the word if you want it unified here too.
- `parse_decimal_notation` is rebuilt on the existing `digits_to_i128` helper, splitting off the sign and the `.` the way `parse_string_to_decimal` does. It returns `Option` so the (unreachable) overflow propagates rather than wrapping, and a new test round-trips every shape ryu emits, including `f64::MAX` and the smallest subnormal.
- Float/double to decimal now reports `Compatible(Some(...))` carrying the JDK < 19 tie-rounding caveat, and the cast audit doc records the same note (it had the same gap).
Verification: `cargo test -p datafusion-comet-spark-expr` 550 passed / 0 failed, clippy and `cargo fmt --check` clean, and the full `CometCastSuite` is 165 passed / 0 failed / 7 ignored.
One unrelated gotcha I hit, in case it saves you time: running a filtered subset of `CometCastSuite` (e.g. `-Dsuites="…CometCastSuite DecimalType"`) makes `cast StringType to DecimalType(38,10) high precision` fail with a Spark-side `java.lang.ArithmeticException: BigInteger would overflow supported range`. `DataGenerator.DEFAULT` is seeded once and shared across the suite, so filtering shifts which random strings that test gets. It reproduces identically without any of my changes and passes in a full-suite run.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org