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: [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]
