breken-ai commented on code in PR #25888:
URL: https://github.com/apache/datafusion/pull/25888#discussion_r4213410856
##########
datafusion/functions-aggregate-common/src/utils.rs:
##########
@@ -120,31 +117,26 @@ impl<T: DecimalType> DecimalAverager<T> {
target_precision: u8,
target_scale: i8,
) -> Result<Self> {
- let sum_mul = T::Native::from_usize(10_usize)
- .map(|b| b.pow_wrapping(sum_scale as u32))
- .ok_or_else(|| {
- internal_datafusion_err!("Failed to compute sum_mul in
DecimalAverager")
- })?;
-
- let target_mul = T::Native::from_usize(10_usize)
- .map(|b| b.pow_wrapping(target_scale as u32))
- .ok_or_else(|| {
- internal_datafusion_err!(
- "Failed to compute target_mul in DecimalAverager"
- )
- })?;
-
- if target_mul >= sum_mul {
- Ok(Self {
- sum_mul,
- target_mul,
- target_precision,
- target_scale,
- })
- } else {
+ // Only the ratio `10^target_scale / 10^sum_scale` is needed, and the
+ // scale difference is non-negative even when both scales are negative
+ // (e.g. `Decimal128(10, -2)`).
+ let scale_diff = i16::from(target_scale) - i16::from(sum_scale);
+ if scale_diff < 0 {
// can't convert the lit decimal to the returned data type
- exec_err!("Arithmetic Overflow in AvgAccumulator")
+ return exec_err!("Arithmetic Overflow in AvgAccumulator");
}
+
+ let Some(scale_mul) = T::Native::from_usize(10_usize)
+ .and_then(|b| b.pow_checked(scale_diff as u32).ok())
Review Comment:
Yes, it did. `pow_checked` returns an `ArrowError` that says what
overflowed, and `.ok()` discarded it. Fixed in ac0e79125: `try_new` now keeps
the error.
```rust
let scale_mul = ten.pow_checked(scale_diff as u32).map_err(|e| {
exec_datafusion_err!(
"Arithmetic Overflow in AvgAccumulator: cannot rescale from scale
{sum_scale} to {target_scale}: {e}"
)
})?;
```
The message still starts with `Arithmetic Overflow in AvgAccumulator`. No
`.slt` or Rust test matches on the old text. The `from_usize(10)` case (can't
happen for i32/i64/i128/i256) is reported as an internal error again, as it was
before this PR.
I also added unit tests in `functions-aggregate-common/src/utils.rs`:
- `decimal_averager_negative_scale`: `Decimal128(10, -2)` → `(14, 2)`
rescales a sum of 300 at scale -2 by 10^4 and averages to 1500000.
- `decimal_averager_unrepresentable_multiplier_keeps_cause`: `try_new(-128,
38, 127)` needs 10^255, which doesn't fit in an i128. The error now reads `...
cannot rescale from scale -128 to 127: Arithmetic overflow: Overflow happened
on: 10 ^ 255`. On the previous head (60067efe) the same test fails with just
`Execution error: Arithmetic Overflow in AvgAccumulator`.
`cargo test -p datafusion-functions-aggregate-common`: 62 passed. `cargo
clippy -p datafusion-functions-aggregate-common --all-targets --all-features --
-D warnings` and `cargo fmt --all -- --check` are clean, and the branch still
merges cleanly into `main`.
--
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]
