drusso commented on a change in pull request #8222: URL: https://github.com/apache/arrow/pull/8222#discussion_r500984291
########## File path: rust/datafusion/src/physical_plan/aggregates.rs ########## @@ -124,14 +126,30 @@ pub fn create_aggregate_expr( let return_type = return_type(&fun, &arg_types)?; - Ok(match fun { - AggregateFunction::Count => { + Ok(match (fun, distinct) { + (AggregateFunction::Count, false) => { Arc::new(expressions::Count::new(arg, name, return_type)) } - AggregateFunction::Sum => Arc::new(expressions::Sum::new(arg, name, return_type)), - AggregateFunction::Min => Arc::new(expressions::Min::new(arg, name, return_type)), - AggregateFunction::Max => Arc::new(expressions::Max::new(arg, name, return_type)), - AggregateFunction::Avg => Arc::new(expressions::Avg::new(arg, name, return_type)), + (AggregateFunction::Count, true) => { + Arc::new(distinct_expressions::DistinctCount::new( + arg_types, + args.clone(), + name, + return_type, + )) + } + (AggregateFunction::Sum, _) => { Review comment: Sounds good, and I will do the same for `Avg`. I will leave `Min` and `Max` as `_` since the `distinct` flag shouldn't have any effect on these aggregations. The only change in behaviour to highlight would be that existing queries that happen to include a `sum(distinct)` or `avg(distinct)` would start to fail, rather than be handled implicitly as a plain `sum()` or `avg()`, but I believe this would be a non-issue. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org