alamb commented on code in PR #7358:
URL: https://github.com/apache/arrow-datafusion/pull/7358#discussion_r1301967419
##########
datafusion/physical-expr/src/aggregate/sum.rs:
##########
@@ -71,18 +63,20 @@ impl Sum {
}
}
-/// Creates a [`PrimitiveGroupsAccumulator`] with the specified
-/// [`ArrowPrimitiveType`] which applies `$FN` to each element
-///
-/// [`ArrowPrimitiveType`]: arrow::datatypes::ArrowPrimitiveType
-macro_rules! instantiate_primitive_accumulator {
- ($SELF:expr, $PRIMTYPE:ident, $FN:expr) => {{
- Ok(Box::new(PrimitiveGroupsAccumulator::<$PRIMTYPE, _>::new(
- &$SELF.data_type,
- $FN,
- )))
- }};
+/// Sum only supports a subset of numeric types, instead relying on type
coercion
Review Comment:
It might help to document what this macro does (aka calls helper macro given
a s (what is s? The aggregate?))
##########
datafusion/physical-expr/src/aggregate/sum.rs:
##########
@@ -123,54 +122,28 @@ impl AggregateExpr for Sum {
}
fn create_groups_accumulator(&self) -> Result<Box<dyn GroupsAccumulator>> {
- // instantiate specialized accumulator
- match self.data_type {
- DataType::UInt64 => {
- instantiate_primitive_accumulator!(self, UInt64Type, |x, y| x
- .add_assign(y))
- }
- DataType::Int64 => {
- instantiate_primitive_accumulator!(self, Int64Type, |x, y| x
- .add_assign(y))
- }
- DataType::UInt32 => {
- instantiate_primitive_accumulator!(self, UInt32Type, |x, y| x
- .add_assign(y))
- }
- DataType::Int32 => {
- instantiate_primitive_accumulator!(self, Int32Type, |x, y| x
- .add_assign(y))
- }
- DataType::Float32 => {
- instantiate_primitive_accumulator!(self, Float32Type, |x, y| x
- .add_assign(y))
- }
- DataType::Float64 => {
- instantiate_primitive_accumulator!(self, Float64Type, |x, y| x
- .add_assign(y))
- }
- DataType::Decimal128(_, _) => {
- instantiate_primitive_accumulator!(self, Decimal128Type, |x,
y| x
- .add_assign(y))
- }
- DataType::Decimal256(_, _) => {
- instantiate_primitive_accumulator!(self, Decimal256Type, |x,
y| *x =
- *x + y)
- }
- _ => not_impl_err!(
- "GroupsAccumulator not supported for {}: {}",
- self.name,
- self.data_type
- ),
+ macro_rules! helper {
+ ($t:ty, $dt:expr) => {
+ Ok(Box::new(PrimitiveGroupsAccumulator::<$t, _>::new(
+ &$dt,
+ |x, y| *x = x.add_wrapping(y),
+ )))
+ };
}
+ downcast_sum!(self, helper)
Review Comment:
the multiple levels of macros is concise I'll give you that but I do find it
hard to follow. Maybe that is ok as we don't expect this to be changing
##########
datafusion/physical-expr/src/aggregate/average.rs:
##########
@@ -93,11 +87,27 @@ impl AggregateExpr for Avg {
}
fn create_accumulator(&self) -> Result<Box<dyn Accumulator>> {
- Ok(Box::new(AvgAccumulator::try_new(
- // avg is f64 or decimal
- &self.input_data_type,
- &self.result_data_type,
- )?))
+ use DataType::*;
+ // instantiate specialized accumulator based for the type
+ match (&self.input_data_type, &self.result_data_type) {
+ (Float64, Float64) => Ok(Box::<AvgAccumulator>::default()),
+ (
+ Decimal128(sum_precision, sum_scale),
+ Decimal128(target_precision, target_scale),
+ ) => Ok(Box::new(DecimalAvgAccumulator {
+ sum: None,
+ count: 0,
+ sum_scale: *sum_scale,
+ sum_precision: *sum_precision,
+ target_precision: *target_precision,
+ target_scale: *target_scale,
+ })),
+ _ => not_impl_err!(
+ "AvgGroupsAccumulator for ({} --> {})",
Review Comment:
```suggestion
"AvgAccumulator for ({} --> {})",
```
--
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]
