Skip to content

Improve GroupBy #1334

Merged
databend-bot merged 9 commits into
databendlabs:masterfrom
sundy-li:group-by3
Aug 8, 2021
Merged

Improve GroupBy #1334
databend-bot merged 9 commits into
databendlabs:masterfrom
sundy-li:group-by3

Conversation

@sundy-li

@sundy-li sundy-li commented Aug 7, 2021

Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/

Summary

  • More generic way in AggregateFunction.
  • Refactor group by, disable take block in the group by, this can avoid lots memcpy, performance can improve 30%~50%.
  • Now we don't need to store "_group_keys" in the partial group by stage, using deserialize from hash key instead.
  • Improve Cast performance in arrow2.
  • Always convert constant column to full column before aggregation.
  • ...

Changelog

  • Improvement

Related Issues

Fixes #1340 #644

Test Plan

Unit Tests

Stateless Tests

@databend-bot databend-bot added pr-bugfix this PR patches a bug in codebase pr-build this PR changes build/testing/ci steps pr-doc-fix pr-feature this PR introduces a new feature to the codebase labels Aug 7, 2021
@databend-bot

Copy link
Copy Markdown
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@sundy-li sundy-li removed pr-bugfix this PR patches a bug in codebase pr-other pr-build this PR changes build/testing/ci steps pr-feature this PR introduces a new feature to the codebase labels Aug 7, 2021
@databendlabs databendlabs deleted a comment from databend-bot Aug 8, 2021
@sundy-li

sundy-li commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

/runperf

@bohutang

bohutang commented Aug 8, 2021

Copy link
Copy Markdown
Member

/run-perf master

@databend-bot

Copy link
Copy Markdown
Member

run performance on sha b7b370b reference on master

@sundy-li

sundy-li commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

GroupBy is 1.7X faster.

https://test-1306375239.cos.accelerate.myqcloud.com/report/1334/b7b370b3e8336ae007a1fa143939675443c9b2f0/36b91cc0-5a19-4256-9644-6c71011831a4/performance.html

@sundy-li

sundy-li commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

/run-perf master

@databend-bot

Copy link
Copy Markdown
Member

run performance on sha 0a0009e reference on master

@sundy-li
sundy-li marked this pull request as ready for review August 8, 2021 10:32
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #1334 (0a0009e) into master (ad6cca6) will decrease coverage by 0%.
The diff coverage is 60%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1334   +/-   ##
======================================
- Coverage      72%     72%   -1%     
======================================
  Files         481     485    +4     
  Lines       28033   28032    -1     
======================================
- Hits        20311   20228   -83     
- Misses       7722    7804   +82     
Impacted Files Coverage Δ
common/datavalues/src/arrays/builders/builder.rs 41% <ø> (-17%) ⬇️
common/datavalues/src/arrays/comparison.rs 37% <ø> (ø)
common/datavalues/src/arrays/kernels/iterator.rs 47% <ø> (ø)
common/datavalues/src/arrays/ops/downcast.rs 65% <ø> (ø)
common/datavalues/src/columns/common.rs 88% <ø> (+41%) ⬆️
common/datavalues/src/data_df_type.rs 11% <ø> (ø)
common/datavalues/src/macros.rs 34% <0%> (-2%) ⬇️
common/datavalues/src/series/arithmetic.rs 63% <ø> (ø)
common/datavalues/src/series/date_wrap.rs 0% <0%> (ø)
common/datavalues/src/utils.rs 24% <ø> (-20%) ⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad6cca6...0a0009e. Read the comment docs.

@sundy-li

sundy-li commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

https://test-1306375239.cos.accelerate.myqcloud.com/report/1334/0a0009ee85293be40899e94a553f9874f3b8265f/3471f870-3f4c-4562-b845-ccaef75733ea/performance.html

GroupBy is nearly 2.0X faster now!

@bohutang bohutang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM
Thank you @sundy-li

@databend-bot

Copy link
Copy Markdown
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
databend-bot merged commit 1137298 into databendlabs:master Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build for Debug Error

4 participants