breken-ai opened a new pull request, #25888: URL: https://github.com/apache/datafusion/pull/25888
## Which issue does this PR close? - Closes #24898. ## Rationale for this change `avg` over a decimal column with a negative scale panics with `attempt to divide by zero` instead of returning a result. For example, averaging `10000` and `20000` stored as `Decimal128(10, -2)` should return `15000.00` as `Decimal128(14, 2)` (the type `Avg::return_type` already declares), but the query task panics. The plain, grouped and `DISTINCT` aggregates all go through the same helper. ## What changes are included in this PR? `DecimalAverager::try_new` previously computed `10^sum_scale` and `10^target_scale` separately with `pow_wrapping(scale as u32)`. A negative scale becomes a huge `u32` exponent, the wrapped power is `0`, and `avg` then divides `target_mul` by that zero `sum_mul`. The two factors were only ever used as the ratio `10^(target_scale - sum_scale)`, so the averager now stores that single multiplier: - the scale difference is computed in `i16`, so it is exact for any pair of `i8` scales; - a negative difference (target scale smaller than the input scale) returns the existing `Arithmetic Overflow in AvgAccumulator` error, as the old `target_mul >= sum_mul` check did; - the power uses `pow_checked`, so an unrepresentable multiplier also returns that error instead of wrapping. `avg` multiplies the sum by the stored multiplier; the precision validation is unchanged. ## What is the testing strategy for this PR? Added `avg_decimal_negative_scale` to `datafusion/sqllogictest/test_files/aggregate.slt`, covering the plain aggregate (value and `Decimal128(14, 2)` result type), a grouped aggregate and `avg(DISTINCT ...)` over `Decimal128(10, -2)` values. - On `main` (`1d9be2e10`) with only the new test: `cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests -- aggregate.slt` fails at the new query with `task 11 panicked with message "attempt to divide by zero"`. - With the fix, the same command passes, and the full `cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests` run passes (524/524 files). - `cargo test --profile ci -p datafusion-functions-aggregate-common -p datafusion-functions-aggregate` passes, `cargo clippy -p datafusion-functions-aggregate-common -p datafusion-functions-aggregate --all-targets --all-features -- -D warnings` is clean, and `cargo fmt --all -- --check` passes. ## Are there any user-facing changes? Yes: `avg` over negative-scale decimals returns the average instead of panicking. No public API changes (the changed fields are private). AI disclosure: this fix was found, written and tested by the breken-ai agent (Breken). Happy to answer questions about the scale arithmetic during review. -- 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]
