Skip to content

Fix bug in TopK aggregates#12766

Merged
avantgardnerio merged 11 commits intoapache:mainfrom
coralogix:bg_topk_proj
Oct 8, 2024
Merged

Fix bug in TopK aggregates#12766
avantgardnerio merged 11 commits intoapache:mainfrom
coralogix:bg_topk_proj

Conversation

@avantgardnerio
Copy link
Copy Markdown
Contributor

@avantgardnerio avantgardnerio commented Oct 4, 2024

Which issue does this PR close?

Closes #12748.

Rationale for this change

We don't want bugs.

What changes are included in this PR?

  1. Push down agg limits past projections so we can observe the bug
  2. Fix the bug

Are these changes tested?

Yes

Are there any user-facing changes?

  1. queries that weren't optimized should now be optimized
  2. queries that are optimized should return correct results

@github-actions github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 4, 2024
@avantgardnerio avantgardnerio changed the title Bg topk proj Fix bug in TopK aggregates Oct 7, 2024
@avantgardnerio avantgardnerio marked this pull request as ready for review October 7, 2024 16:15
@avantgardnerio avantgardnerio requested a review from alamb October 7, 2024 16:15
@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Oct 7, 2024
Dandandan
Dandandan reviewed Oct 7, 2024
View reviewed changes
Comment thread datafusion/physical-plan/src/execution_plan.rs Outdated
thinkharderdev
thinkharderdev reviewed Oct 7, 2024
View reviewed changes
Comment thread datafusion/physical-plan/src/sorts/sort.rs
Comment thread datafusion/physical-plan/src/execution_plan.rs
avantgardnerio and others added 2 commits October 7, 2024 12:49
@avantgardnerio @thinkharderdev
Docs
d18c05b 
Co-authored-by: Dan Harris <1327726+thinkharderdev@users.noreply.github.com>
@avantgardnerio
PR feedback
369045d
Dandandan
Dandandan approved these changes Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Dandandan Dandandan 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 to me 🚀

thinkharderdev
thinkharderdev approved these changes Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@thinkharderdev thinkharderdev left a comment

Choose a reason for hiding this comment

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

lgtm!

@avantgardnerio avantgardnerio merged commit c412c74 into apache:main Oct 8, 2024
@avantgardnerio avantgardnerio deleted the bg_topk_proj branch October 8, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dandandan Dandandan Dandandan approved these changes

@thinkharderdev thinkharderdev thinkharderdev approved these changes

@alamb alamb Awaiting requested review from alamb

@andygrove andygrove Awaiting requested review from andygrove

Assignees

No one assigned

Labels

optimizer Optimizer rules physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Probable bug in TopKAggregate

3 participants

@avantgardnerio @Dandandan @thinkharderdev
⚡