Skip to content

fix: keep EliminateGroupByConstant from requalifying group expressions - #26356

Open
adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:fix/eliminate-group-by-constant-requalify
Open

adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:fix/eliminate-group-by-constant-requalify

Conversation

@adriangb

Copy link
Copy Markdown
Contributor

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.

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.

@github-actions github-actions Bot added the optimizer Optimizer rules label Oct 15, 2026
adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 15, 2026
@codecov-commenter

codecov-commenter commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.70588% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.89%. Comparing base (e732212) to head (2a7c2fa).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
...usion/optimizer/src/eliminate_group_by_constant.rs 89.70% 3 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26356      +/-   ##
==========================================
+ Coverage   82.83%   82.89%   +0.05%     
==========================================
  Files        1151     1153       +2     
  Lines      455332   457542    +2210     
  Branches   455332   457542    +2210     
==========================================
+ Hits       377165   379257    +2092     
- Misses      55262    55272      +10     
- Partials    22905    23013     +108     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Removing aliases from redundant group keys discards their output field metadata.

1 open finding
What changed in this PR

Fixes EliminateGroupByConstant incorrectly requalifying group expressions when rebuilding an aggregate’s output projection.

Changes:

  • Builds the projection directly from aggregate output columns.
  • Leaves grouping-set aggregates unchanged.
  • Adds a regression test for unqualified references to qualified inputs.
File Description
datafusion/​optimizer/​src/​eliminate_group_by_constant.rs Updates projection construction, guards grouping sets, and adds regression coverage.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread datafusion/optimizer/src/eliminate_group_by_constant.rs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused rewrite addresses the qualification failure with regression coverage and no identified blocking issues; tests were not rerun during review.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants