Skip to content

fix: grouping with alias#21438

Merged
timsaucer merged 7 commits into
apache:mainfrom
timsaucer:bugfix/grouping-with-alias
Apr 12, 2026
Merged

fix: grouping with alias#21438
timsaucer merged 7 commits into
apache:mainfrom
timsaucer:bugfix/grouping-with-alias

Conversation

@timsaucer

@timsaucer timsaucer commented Apr 7, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

When you have an alias on grouping function via dataframe API, you get an error. This resolves that error.

What changes are included in this PR?

Check for alias expressions in optimizer.
Add unit tests.

Are these changes tested?

Unit test added, including a note on why a SQL logic test will not cover this case.

Are there any user-facing changes?

None

timsaucer and others added 2 commits April 6, 2026 16:42
@timsaucer
Check for alias expressions when applying ResolveGroupingFunction
dc814ff
@timsaucer @claude
Remove invalid SQL regression test and document why DataFrame API tes…
9daa3c7 
…t is needed

The SQL planner handles aliasing via projection, so ResolveGroupingFunction
never sees Expr::Alias in SQL queries. Only the DataFrame API produces that
expression shape, making the SQL test insufficient as a regression test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate labels Apr 7, 2026
@timsaucer
timsaucer marked this pull request as ready for review April 7, 2026 12:19
@timsaucer
timsaucer requested a review from Copilot April 7, 2026 12:42
Copilot AI reviewed Apr 7, 2026
View reviewed changes

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.

Pull request overview

Fixes DataFrame API aggregation planning when grouping() is wrapped in an Expr::Alias, aligning behavior with SQL planning and preventing a physical planning error for GROUPING.

Changes:

  • Extend ResolveGroupingFunction to recognize grouping() when wrapped in Expr::Alias.
  • Update grouping-function detection to recurse through alias nodes.
  • Add a DataFrame regression test covering grouping(col).alias(...) in aggregates.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
datafusion/optimizer/src/analyzer/resolve_grouping_function.rs Teach the analyzer rule to detect/replace aliased grouping() expressions.
datafusion/core/tests/dataframe/mod.rs Add regression coverage for aliased grouping() via the DataFrame API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread datafusion/optimizer/src/analyzer/resolve_grouping_function.rs Outdated
Comment thread datafusion/core/tests/dataframe/mod.rs
adriangb
adriangb approved these changes Apr 11, 2026
View reviewed changes
Comment thread datafusion/core/tests/dataframe/mod.rs
/// around the aggregate function in SQL queries — making SQL-based tests
/// insufficient to cover this case.
#[tokio::test]
async fn test_grouping_with_alias() -> Result<()> {

@adriangb adriangb Apr 11, 2026

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.

I confirmed this test fails on main.

@timsaucer
timsaucer added this pull request to the merge queue Apr 12, 2026
Merged via the queue into apache:main with commit d68373e Apr 12, 2026
35 checks passed
@timsaucer
timsaucer deleted the bugfix/grouping-with-alias branch April 12, 2026 13:58
coderfender pushed a commit to coderfender/datafusion that referenced this pull request Apr 14, 2026
@timsaucer @claude
fix: grouping with alias (apache#21438)
a11ad50 
## Which issue does this PR close?

- Closes apache#21411

## Rationale for this change

When you have an alias on `grouping` function via dataframe API, you get
an error. This resolves that error.

## What changes are included in this PR?

Check for alias expressions in optimizer.
Add unit tests.

## Are these changes tested?

Unit test added, including a note on why a SQL logic test will not cover
this case.

## Are there any user-facing changes?

None

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@timsaucer @claude @Rich-T-kid
fix: grouping with alias (apache#21438)
6544367 
## Which issue does this PR close?

- Closes apache#21411

## Rationale for this change

When you have an alias on `grouping` function via dataframe API, you get
an error. This resolves that error.

## What changes are included in this PR?

Check for alias expressions in optimizer.
Add unit tests.

## Are these changes tested?

Unit test added, including a note on why a SQL logic test will not cover
this case.

## Are there any user-facing changes?

None

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fangchenli added a commit to fangchenli/datafusion that referenced this pull request Jul 9, 2026
@fangchenli @claude
fix: apply single_distinct_to_groupby to aliased DataFrame API aggreg…
de18cf2 
…ates

The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a
nested double aggregate, but only fired for SQL-planned plans. The DataFrame
API applies the output alias directly to the aggregate expression, so
aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only
matched a bare Expr::AggregateFunction and the rule never fired, leaving the
DataFrame path materially slower than the equivalent SQL.

Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite
(aliases nested inside sub-expressions are left untouched). Output column
names are preserved because the rule already rebuilds the outer projection
from the original aggregate's schema. Mirrors the fix for the same bug class
in apache#21411 / apache#21438.

Closes apache#23401.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli added a commit to fangchenli/datafusion that referenced this pull request Jul 9, 2026
@fangchenli @claude
fix: apply single_distinct_to_groupby to aliased DataFrame API aggreg…
1aa3e4c 
…ates

The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a
nested double aggregate, but only fired for SQL-planned plans. The DataFrame
API applies the output alias directly to the aggregate expression, so
aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only
matched a bare Expr::AggregateFunction and the rule never fired, leaving the
DataFrame path materially slower than the equivalent SQL.

Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite
(aliases nested inside sub-expressions are left untouched). Output column
names are preserved because the rule already rebuilds the outer projection
from the original aggregate's schema. Mirrors the fix for the same bug class
in apache#21411 / apache#21438.

Closes apache#23401.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli added a commit to fangchenli/datafusion that referenced this pull request Jul 9, 2026
@fangchenli @claude
fix: apply single_distinct_to_groupby to aliased DataFrame API aggreg…
811088a 
…ates

The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a
nested double aggregate, but only fired for SQL-planned plans. The DataFrame
API applies the output alias directly to the aggregate expression, so
aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only
matched a bare Expr::AggregateFunction and the rule never fired, leaving the
DataFrame path materially slower than the equivalent SQL.

Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite
(aliases nested inside sub-expressions are left untouched). Output column
names are preserved because the rule already rebuilds the outer projection
from the original aggregate's schema. Mirrors the fix for the same bug class
in apache#21411 / apache#21438.

Closes apache#23401.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli added a commit to fangchenli/datafusion that referenced this pull request Jul 9, 2026
@fangchenli @claude
fix: apply single_distinct_to_groupby to aliased DataFrame API aggreg…
c388b94 
…ates

The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a
nested double aggregate, but only fired for SQL-planned plans. The DataFrame
API applies the output alias directly to the aggregate expression, so
aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only
matched a bare Expr::AggregateFunction and the rule never fired, leaving the
DataFrame path materially slower than the equivalent SQL.

Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite
(aliases nested inside sub-expressions are left untouched). Output column
names are preserved because the rule already rebuilds the outer projection
from the original aggregate's schema. Mirrors the fix for the same bug class
in apache#21411 / apache#21438.

Also skip the rewrite when a DISTINCT argument is a constant. Such an argument
would become a constant GROUP BY key that eliminate_group_by_constant collapses
into a global aggregate, which emits a row even on empty input and produces
wrong results (e.g. MIN(DISTINCT 37) over empty input returned 37, not NULL).
This is a latent bug the alias fix would otherwise expose more widely; the
rewrite has no benefit for a constant argument anyway.

Closes apache#23401.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli added a commit to fangchenli/datafusion that referenced this pull request Jul 9, 2026
@fangchenli @claude
fix: apply single_distinct_to_groupby to aliased DataFrame API aggreg…
7975288 
…ates

The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a
nested double aggregate, but only fired for SQL-planned plans. The DataFrame
API applies the output alias directly to the aggregate expression, so
aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only
matched a bare Expr::AggregateFunction and the rule never fired, leaving the
DataFrame path materially slower than the equivalent SQL.

Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite
(aliases nested inside sub-expressions are left untouched). Output column
names are preserved because the rule already rebuilds the outer projection
from the original aggregate's schema. Mirrors the fix for the same bug class
in apache#21411 / apache#21438.

Also skip the rewrite when a DISTINCT argument is a constant. Such an argument
would become a constant GROUP BY key that eliminate_group_by_constant collapses
into a global aggregate, which emits a row even on empty input and produces
wrong results (e.g. MIN(DISTINCT 37) over empty input returned 37, not NULL).
This is a latent bug the alias fix would otherwise expose more widely; the
rewrite has no benefit for a constant argument anyway.

Closes apache#23401.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@adriangb adriangb adriangb approved these changes

Assignees

No one assigned

Labels

core Core DataFusion crate optimizer Optimizer rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ResolveGroupingFunction does not unwrap Alias nodes

3 participants

@timsaucer @adriangb