alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3508066505
##########
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:
this basically assumes that any function with the name count() will behave
as the built in count 🤔 that doesn't seem safe to me (unless it is done
elsewhere)
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,26 +60,42 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
Review Comment:
these plans are changing but I don't see the actual query results changing.
Is that expected?
##########
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")?;
+ let sentinel = count_udaf
+ .call(vec![lit(1_i64)])
+ .alias(sentinel_name.as_str());
+ let mut aggr_with_sentinel = aggregate.aggr_expr.clone();
Review Comment:
Can we avoid the clone
--
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]
