AdamGS commented on code in PR #25888:
URL: https://github.com/apache/datafusion/pull/25888#discussion_r4148028288
##########
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:
doesn't the `ok()` here loses some error information?
--
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]
