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