-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: update group by columns for merge phase after spill #15531
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.
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
Conversation
3fabcc4
to
ae94ef4
Compare
The CI failures are infra related... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @rluvaton
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rluvaton and @comphead
FYI @2010YOUY01 who is working on spilling and @korowa who worked on the code in question
Thank you @rluvaton |
* fix: update group by columns for merge phase after spill fixes apache#15530 * Update datafusion/physical-plan/src/aggregates/row_hash.rs Co-authored-by: Oleks V <comphead@users.noreply.github.com> * move test to aggregate_fuzz --------- Co-authored-by: Oleks V <comphead@users.noreply.github.com>
Which issue does this PR close?
AggregateMode::Single
#15530.Rationale for this change
I think the PR below forgot to update the group by expressions:
What changes are included in this PR?
Update group by columns to be based on their index for merging phase
Are these changes tested?
Yes
Are there any user-facing changes?
No