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