adriangb opened a new pull request, #26356:
URL: https://github.com/apache/datafusion/pull/26356
## Which issue does this PR close?
No issue filed. The reproduction is below.
## Rationale for this change
`EliminateGroupByConstant` re-plans the original group expressions over the
new `Aggregate` with `LogicalPlanBuilder::project`. Column normalization falls
back to the input schema of the aggregate. Thus an unqualified group
expression, such as an extracted leaf `__datafusion_extracted_1`, becomes
`m.__datafusion_extracted_1` when the input is a `SubqueryAlias: m`. The new
aggregate does not output that name, and the rule fails:
```
Optimizer rule 'eliminate_group_by_constant' failed
caused by
Schema error: No field named m.__datafusion_extracted_1.
```
The input plan is valid, and the rule turns it into an invalid one. We reach
it when we optimize a plan, bind its parameters with
`replace_params_with_values`, and optimize again. The bound value makes a group
key redundant only after `ExtractLeafExpressions` has rewritten the plan. In a
single `optimize` call, `EliminateGroupByConstant` runs before
`ExtractLeafExpressions` in each pass, so I could not reproduce it there. Since
#26184, the arguments of filtered aggregates no longer go into a
common-expression projection between the aggregate and the alias, so the
aggregate can sit directly on the alias and the problem shows.
```rust
let ctx = SessionContext::new();
ctx.sql("CREATE TABLE t AS SELECT * FROM (VALUES (TIMESTAMP
'2026-01-01T00:00:00', named_struct('a', 'x'), 1)) AS v(ts, s, x)")
.await?.collect().await?;
let scan = ctx.table("t").await?.into_unoptimized_plan();
let plan = LogicalPlanBuilder::from(scan)
.alias("m")?
.aggregate(
vec![
date_bin(
cast(placeholder("$1"),
DataType::Interval(IntervalUnit::MonthDayNano)),
col("ts"),
lit(ScalarValue::TimestampNanosecond(Some(0), None)),
),
get_field(col("s"), "a").alias("c"),
col("ts"),
],
vec![
sum(col("x") + lit(1)).filter(col("x").gt(lit(0))).build()?,
max(col("x") + lit(1)).filter(col("x").lt(lit(5))).build()?,
],
)?
.build()?;
let state = ctx.state();
let plan = state.optimize(&plan)?;
let plan = plan.replace_params_with_values(&ParamValues::List(vec![
ScalarValue::Utf8(Some("1 minute".into())).into(),
]))?;
state.optimize(&plan)?; // fails with the error above
```
## What changes are included in this PR?
- The rule builds the projection with `Projection::try_new` and does not
re-plan the group expressions. A kept key or an aggregate becomes a column of
the schema of the new aggregate. A redundant key keeps its expression, which
reads only bare GROUP BY columns that the new aggregate outputs unchanged. An
item gets an alias only when its name differs from the original output field,
so existing plans do not change.
- The rule leaves an aggregate with a `GroupingSet` unchanged, because its
output fields do not map one to one to `group_expr`.
## What is the testing strategy for this PR?
- New unit test
`test_eliminate_with_unqualified_reference_to_qualified_input`. It fails
without the change, with the same schema error.
- `cargo test -p datafusion-optimizer` and `cargo test -p
datafusion-sqllogictest --test sqllogictests` pass. No existing plan changes.
## Are there any user-facing changes?
No API changes. A plan that failed in this rule now optimizes.
--
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]
