alamb commented on code in PR #10648:
URL: https://github.com/apache/datafusion/pull/10648#discussion_r1617221359
##########
datafusion/optimizer/src/replace_distinct_aggregate.rs:
##########
@@ -99,17 +97,7 @@ impl OptimizerRule for ReplaceDistinctWithAggregate {
// Construct the aggregation expression to be used to fetch
the selected expressions.
let aggr_expr = select_expr
.into_iter()
- .map(|e| {
- Expr::AggregateFunction(AggregateFunction::new(
- AggregateFunctionFunc::FirstValue,
- vec![e],
- false,
- None,
- sort_expr.clone(),
- None,
- ))
- })
- .collect::<Vec<Expr>>();
+ .map(|e| first_value(vec![e], false, None,
sort_expr.clone(), None));
Review Comment:
Perhaps what @jayzhan211 is noticing is that by directly relying on
`datafusion-aggregate-functions` in the optimizer it means that now the
optimizer is still treating "built in" aggregates specially (and for example,
user defined functions couldn't take advantage of this)
So if we moved this rule (or somehow made the dependency via an interface)
it would be more general and decouple the code and make datafusion more
extensible.
One way is to move the entire rule into `datafusion-aggregate-functions` but
maybe we could instead have `replace_distinct_aggregate` depend on
`FunctionRegistry` rather than a direct call to `first_value` for example
Maybe we can file a TODO ticket to track it
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
