kosiew commented on code in PR #22813:
URL: https://github.com/apache/datafusion/pull/22813#discussion_r3379310519
##########
datafusion/spark/src/function/math/round.rs:
##########
@@ -187,20 +190,43 @@ fn get_scale(args: &[ColumnarValue]) ->
Result<Option<i32>> {
/// round_float(125.0, -1) → 130.0
/// ```
fn round_float<T: num_traits::Float>(value: T, scale: i32) -> T {
- if scale >= 0 {
- let factor = T::from(10.0f64.powi(scale)).unwrap_or_else(T::infinity);
- if factor.is_infinite() {
- // Very large positive scale — value is already precise enough,
return as-is
- return value;
- }
- (value * factor).round() / factor
- } else {
- let factor = T::from(10.0f64.powi(-scale)).unwrap_or_else(T::infinity);
- if factor.is_infinite() {
- // Very large negative scale — any finite value rounds to 0
- return T::zero();
- }
- (value / factor).round() * factor
+ // Widen to f64 first. For f32 inputs this matches Spark's `f.toDouble`
+ // step (FloatType: `BigDecimal(f.toDouble).setScale(..).toFloat`), which
+ // exposes the binary-float error before rounding. For f64 it is a no-op.
+ let Some(d) = value.to_f64() else {
+ return value;
+ };
+
+ // Spark returns NaN / ±Inf unchanged; BigDecimal cannot represent them.
+ if !d.is_finite() {
+ return value;
+ }
+
+ // `d.to_string()` produces the shortest round-trip decimal string,
matching
+ // Scala's `BigDecimal(d) = java.math.BigDecimal.valueOf(d)` semantics. So
+ // `round(1.255_f64, 2)` parses "1.255" and rounds to 1.26 (not the naive
+ // binary-float 1.25).
+ let Ok(bd) = BigDecimal::from_str(&d.to_string()) else {
Review Comment:
Since we've already guarded against non-finite `f64` values,
`BigDecimal::from_str(&d.to_string())` should always succeed. Rust's display
output for a finite float is valid decimal input for `BigDecimal`.
Returning the original value here makes that invariant a little less
explicit and could potentially hide a future regression. Would it make sense to
encode the assumption directly with something like:
```rust
let bd = BigDecimal::from_str(&d.to_string())
.expect("finite f64 Display parses as BigDecimal");
```
Alternatively, a `debug_assert!` plus an explicit fallback could work if
panicking is not desirable on this path.
--
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]
