Skip to content

Fix grouping set subset satisfaction #19853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
gabotechs merged 4 commits into from
Jan 16, 2026
Merged

Fix grouping set subset satisfaction #19853

gabotechs merged 4 commits into from
Jan 16, 2026
+263 −2

Conversation

@freakyzoidberg
Copy link
Member

@freakyzoidberg freakyzoidberg commented Jan 16, 2026

Summary

  • Fixes incorrect results from ROLLUP/CUBE/GROUPING SETS queries when using multiple partitions
  • The subset satisfaction optimization was incorrectly allowing hash partitioning on fewer columns to satisfy requirements that include __grouping_id
  • This caused partial aggregates from different partitions to be finalized independently, producing duplicate grand totals

Closes #19849

@github-actions github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jan 16, 2026
@gabotechs
Copy link
Contributor

gabotechs commented Jan 16, 2026 •
edited
Loading

cc @gene-bordegaray do this changes look good to you?

@gabotechs
Copy link
Contributor

gabotechs commented Jan 16, 2026

I can confirm that this change does fix the issue, I'll let @gene-bordegaray chime in as the one assigned to the original issue.

Unless there are any relevant comments, I'll merge this in as soon as CI passes.

@gene-bordegaray
Copy link
Contributor

gene-bordegaray commented Jan 16, 2026 •
edited
Loading

Yes this looks correct, thanks @freakyzoidberg . Let me unassign myself and comment take on issue

only thing may be wanting to add plans in the sqllogictests 😄

cc: @gabotechs

gabotechs
gabotechs reviewed Jan 16, 2026
View reviewed changes
datafusion/sqllogictest/test_files/grouping_set_repartition.slt
Comment on lines +145 to +147
04)------AggregateExec: mode=FinalPartitioned, gby=[channel@0 as channel, brand@1 as brand, __grouping_id@2 as __grouping_id], aggr=[sum(sub.total)]
05)--------RepartitionExec: partitioning=Hash([channel@0, brand@1, __grouping_id@2], 4), input_partitions=4
06)----------AggregateExec: mode=Partial, gby=[(NULL as channel, NULL as brand), (channel@0 as channel, NULL as brand), (channel@0 as channel, brand@1 as brand)], aggr=[sum(sub.total)]
Copy link
Contributor

@gabotechs gabotechs Jan 16, 2026

Choose a reason for hiding this comment

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

👍 I imagine before this PR the RepartitionExec would not be there right?

Copy link
Member Author

@freakyzoidberg freakyzoidberg Jan 16, 2026

Choose a reason for hiding this comment

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

Correct

gabotechs
gabotechs approved these changes Jan 16, 2026
View reviewed changes
Copy link
Contributor

@gabotechs gabotechs left a comment

Choose a reason for hiding this comment

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

👍 Looks good! thanks @freakyzoidberg and @gene-bordegaray

@gabotechs gabotechs added this pull request to the merge queue Jan 16, 2026
Merged via the queue into apache:main with commit 1ab7e41 Jan 16, 2026
32 checks passed
gabotechs pushed a commit to gabotechs/datafusion that referenced this pull request Jan 16, 2026
@freakyzoidberg @gabotechs
Fix grouping set subset satisfaction (apache#19853)
6caf352 
## Summary
- Fixes incorrect results from ROLLUP/CUBE/GROUPING SETS queries when
using multiple partitions
- The subset satisfaction optimization was incorrectly allowing hash
partitioning on fewer columns to satisfy requirements that include
`__grouping_id`
- This caused partial aggregates from different partitions to be
finalized independently, producing duplicate grand totals
  
Closes apache#19849
This was referenced Jan 16, 2026
Merged
Open
@alamb
Copy link
Contributor

alamb commented Jan 16, 2026

Love it -- thank you

@alamb
Copy link
Contributor

alamb commented Jan 16, 2026 •
edited
Loading

Thank you @freakyzoidberg and @gabotechs

Do you we know what changes in 52 introduced this problem?

@gabotechs
Copy link
Contributor

gabotechs commented Jan 16, 2026

You have the full conclusion here #19849

gabotechs added a commit that referenced this pull request Jan 16, 2026
@gabotechs @freakyzoidberg
[branch 52] Fix grouping set subset satisfaction (#19855)
eb00fe2 
Brings #19853 into `branch-52`

Co-authored-by: Pierre Lacave <pierre.lacave@datadoghq.com>
@alamb
Copy link
Contributor

alamb commented Jan 16, 2026

Thanks @gabotechs -- I'll ask some follow up questions there

NGA-TRAN
NGA-TRAN reviewed Jan 17, 2026
View reviewed changes
Copy link
Contributor

@NGA-TRAN NGA-TRAN left a comment

Choose a reason for hiding this comment

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

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabotechs gabotechs gabotechs approved these changes

+1 more reviewer

@NGA-TRAN NGA-TRAN NGA-TRAN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Different results in TPC-DS q14 depending on the number of partitions

5 participants

@freakyzoidberg @gabotechs @gene-bordegaray @alamb @NGA-TRAN