HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3583218565
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,23 +66,52 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr,
&group_by_columns));
-
if redundant.is_empty()
|| (required.is_empty() && aggregate.aggr_expr.is_empty())
{
return
Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
}
- let simplified_aggregate =
LogicalPlan::Aggregate(Aggregate::try_new(
- aggregate.input,
- required.into_iter().cloned().collect(),
- aggregate.aggr_expr.clone(),
- )?);
-
- let projection_expr =
-
aggregate.group_expr.into_iter().chain(aggregate.aggr_expr);
+ let simplified_plan = if required.is_empty() {
+ let Some(registry) = config.function_registry() else {
+ return
Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
+ };
+ let sentinel_name =
+ config.alias_generator().next("__row_count_sentinel");
+ let count_udaf = registry.udaf("count")?;
Review Comment:
`replace_distinct_aggregate.rs` line 134 does something similar with `let
first_value_udaf: Arc<datafusion_expr::AggregateUDF> =
config.function_registry().unwrap().udaf("first_value")?;`
Not saying that it's the right way to go just that it exists elsewhere.
Otherwise, we could import count_udaf() from datafusion-functions-aggregate.
--
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]
