Skip to content
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

Aggregate partition mode #6938

Closed
wants to merge 26 commits into from
Closed

Aggregate partition mode #6938

wants to merge 26 commits into from

Conversation

Dandandan
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb and others added 25 commits July 10, 2023 12:46
@alamb
Vectorized hash grouping
54a5c95
@alamb
Prepare for merge to main
1991a76
@alamb
Improve comments and update size calculations
8464816
@alamb
Implement test for accumulate_boolean
4bd3066 
refactor
@alamb
Use resize instead of resize_with
7c97b24
@alamb
fix avg size calculation
3ca27ac
@alamb
Simplify sum accumulator
e4a52f9
@alamb
Add comments explaining i64 as counts
f9eaa68
@alamb
Clarify aggreate_arguments
fc96b13
@alamb @mustafasrepo
Apply suggestions from code review
9db6f4b 
Co-authored-by: Mustafa Akur <106137913+mustafasrepo@users.noreply.github.com>
@alamb
Merge remote-tracking branch 'apache/main' into alamb/fast_gby_hash
edc8c43
@alamb
Merge branch 'alamb/fast_gby_hash' of github.com:alamb/arrow-datafusi…
19b8981 
…on into alamb/fast_gby_hash
@alamb
Clarify rationale for ScratchSpace being a field
90f8730
@alamb
use slice syntax
3369ec1
@alamb
Merge remote-tracking branch 'apache/main' into alamb/fast_gby_hash
4124bfa
@alamb @mustafasrepo
Update datafusion/physical-expr/src/aggregate/average.rs
58e3b6d 
Co-authored-by: Mustafa Akur <106137913+mustafasrepo@users.noreply.github.com>
@alamb @mustafasrepo
Update datafusion/physical-expr/src/aggregate/count.rs
47135ba 
Co-authored-by: Mustafa Akur <106137913+mustafasrepo@users.noreply.github.com>
@alamb @mustafasrepo
Update datafusion/physical-expr/src/aggregate/groups_accumulator/adap…
744b4aa 
…ter.rs

Co-authored-by: Mustafa Akur <106137913+mustafasrepo@users.noreply.github.com>
@alamb
fix diagram
c3d5ff2
@alamb @mustafasrepo
Update datafusion/physical-expr/src/aggregate/groups_accumulator/adap…
92f6234 
…ter.rs

Co-authored-by: Mustafa Akur <106137913+mustafasrepo@users.noreply.github.com>
@alamb
Merge branch 'alamb/fast_gby_hash' of github.com:alamb/arrow-datafusi…
f35f2ae 
…on into alamb/fast_gby_hash
@alamb
simplify the supported logic
d19c41e
@alamb
Add a log message when using slow adapter
da911a3
@alamb
fmt
de7b250
Add partition aggregation mode
94ac9d1
@github-actions github-actions bot added physical-expr Physical Expressions core Core DataFusion crate labels Jul 12, 2023
@alamb
Copy link
Contributor

alamb commented Jul 12, 2023

Not sure if this is related to #6937 but I figured I would point it out

@Dandandan
Copy link
Contributor Author

Dandandan commented Jul 12, 2023 •
edited
Loading

@alamb
this is just some WIP on top of #6904 to implement #6892

For this change I'm seeing some positive and some negative changes (timing wise).
Memory wise will likely be better as there is one fewer aggregate (so doesn't need to keep the higher-cardinality states in memory of the partial aggregate), but it seems the RepartitionExec at this point can be slower than the partial aggregate + RepartitionExec 🤔 I think we might have to transfer some of the learnings from the hash aggregate to hash-repartitioning.

@Dandandan
Copy link
Contributor Author

Cleaning to keep list of PRs clean

@Dandandan Dandandan closed this Jul 12, 2023
@Dandandan Dandandan mentioned this pull request Jul 13, 2023
Closed
@Dandandan Dandandan deleted the aggregate_partition_mode branch August 12, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core Core DataFusion crate physical-expr Physical Expressions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Dandandan @alamb