Uh oh!
There was an error while loading. Please reload this page.
ARROW-11290: [Rust][DataFusion] Address hash aggregate performance issue with high number of groups - #9234
Closed
Dandandan wants to merge 2 commits into
Closed
ARROW-11290: [Rust][DataFusion] Address hash aggregate performance issue with high number of groups#9234Dandandan wants to merge 2 commits into
Dandandan wants to merge 2 commits into
Conversation
Codecov Report
@@ Coverage Diff @@## master #9234 +/- ##
=======================================
Coverage 81.61% 81.61% =======================================
Files 215 215 Lines 51891 51897 +6 =======================================
+ Hits 42353 42358 +5 - Misses 9538 9539 +1
Continue to review full report at Codecov.
|
jorgecarleitao
approved these changes
Jan 17, 2021
jorgecarleitao
left a comment
Member
There was a problem hiding this comment.
Cool idea. LGTM. Thanks a lot, @Dandandan
| create_accumulators(aggr_expr).map_err(DataFusionError::into_arrow_external_error)?; | ||
| // Keys received in this batch | ||
| let mut batch_keys = vec![]; |
Member
There was a problem hiding this comment.
Should this be a set rather than a vec since it is intended to track the unique set of keys in the batch?
ContributorAuthor
There was a problem hiding this comment.
That's what I thought first, but this is checked already when pushing the keys to the vec.
ContributorAuthor
There was a problem hiding this comment.
It checks for either an empty indices vec (which means no rows yet with this key) or being the first row with this key in or_insert_with.
andygrove
approved these changes
Jan 17, 2021
Dandandan
commented
Jan 18, 2021
ContributorAuthor
Added a ticket for remaining work including some profiling information here: https://issues.apache.org/jira/browse/ARROW-11300 |
alamb
commented
Jan 20, 2021
Contributor
I merged this branch locally to master and re-ran all the tests. Things looked good so merging it in. |
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
…sue with high number of groups Currently, we loop to the hashmap for every key. However, as we receive a batch, if we a lot of groups in the group by expression (or receive sorted data, etc.) then we could create a lot of empty batches and call `update_batch` for each of the key already in the hashmap. In the PR we keep track of which keys we received in the batch and only update the accumulators with the same keys instead of all accumulators. On the db-benchmark h2oai/db-benchmark#182 this is the difference (mainly q3 and q5, others seem to be noise). It doesn't seem to completely solve the problem, but it reduces the problem already quite a bit. This PR: ``` q1 took 340 ms q2 took 1768 ms q3 took 10975 ms q4 took 337 ms q5 took 13529 ms ``` Master: ``` q1 took 330 ms q2 took 1648 ms q3 took 16408 ms q4 took 335 ms q5 took 21074 ms ``` Closes#9234 from Dandandan/hash_agg_speed2 Authored-by: Heres, Daniel <danielheres@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
jorgecarleitao pushed a commit
that referenced
this pull request
Jan 28, 2021
…ash aggregation with small groups Based on #9234, this PR improves the situation described in https://issues.apache.org/jira/browse/ARROW-11300. The current situation is that we call `take` on arrays, which is fine, but causes a lot of small `Arrays` to be created / allocated. when we have only a small number of rows in each group. This improves the results on the group by queries on db-benchmark: PR: ``` q1 took 32 ms q2 took 422 ms q3 took 3468 ms q4 took 44 ms q5 took 3166 ms q7 took 3081 ms ``` #9234 (different results from that PR description as this has now partitioning enabled and a custom allocator) ``` q1 took 34 ms q2 took 389 ms q3 took 4590 ms q4 took 47 ms q5 took 5152 ms q7 took 3941 ms ``` The PR changes the algorithm to: * Create indices / offsets of all keys / indices new in the batch. * `take` the arrays based on indices in one go (so it only requires one bigger allocation for each array) * Use `slice` based on the offsets to take values from the arrays and pass it to the accumulators. Closes#9271 from Dandandan/hash_agg_few_rows Authored-by: Heres, Daniel <danielheres@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
This was referenced Jan 20, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, we loop to the hashmap for every key.
However, as we receive a batch, if we a lot of groups in the group by expression (or receive sorted data, etc.) then we could create a lot of empty batches and call
update_batchfor each of the key already in the hashmap.In the PR we keep track of which keys we received in the batch and only update the accumulators with the same keys instead of all accumulators.
On the db-benchmark h2oai/db-benchmark#182 this is the difference (mainly q3 and q5, others seem to be noise). It doesn't seem to completely solve the problem, but it reduces the problem already quite a bit.
This PR:
Master: