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

executor: Improve the performance of aggFuncSum by using sliding window #14887

Merged
merged 27 commits into from
Mar 23, 2020
Merged

executor: Improve the performance of aggFuncSum by using sliding window #14887

merged 27 commits into from
Mar 23, 2020

Conversation

mmyj
Copy link
Member

@mmyj mmyj commented Feb 21, 2020 •
edited by SunRunAway
Loading

What problem does this PR solve?

part of #12967

1 Fix a bug of windowFuncBenchmark, It causes the arg ndv, the count of distinct group-by keys, can not work.
2 Modify the rangeFrameWindowProcessor.appendResult2Chunk by using sliding window.
3 Implement the aggFuncSum using sliding window.
4 Add a new variable windowing_use_high_precision.

Check List

Tests

  • Unit test
    executor/window_test.go:222 TestSlidingWindowFunctions

Code changes

1 The partialResult4Count of the aggFuncCount, using a struct saves the partialResult like the other aggFunc, like aggFuncSum.
2 Add a partialResult named slidingWindowVal for partialResult, it used for saving the partialResult of the slidingWindow
3 Abort the WindowProcessor.appendResult2Chunk, we need use the ResetSlidePartialResult to reset the partialResult of the slidingWindow after a sliding window are evaluated completely.

Side effects

  • benchmark
rows
WindowFunctionsWithSlidingWindow/(func:sum,_aggColType:float,_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)-16             21.7ms ± 8%   6.8ms ± 5%  -68.44%  (p=0.000 n=10+10)
WindowFunctionsWithSlidingWindow/(func:sum,_aggColType:decimal(11,0),_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)-16     74.4ms ± 3%  12.3ms ± 5%  -83.46%  (p=0.000 n=10+10)
WindowFunctionsWithSlidingWindow/(func:count,_aggColType:int(11),_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)-16         21.7ms ± 4%   7.0ms ± 2%  -67.85%  (p=0.000 n=10+9)

ranges
WindowFunctionsWithSlidingWindow/(func:sum,_aggColType:float,_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)#01-16           757ms ± 6%     7ms ± 4%  -99.02%  (p=0.000 n=10+10)
WindowFunctionsWithSlidingWindow/(func:sum,_aggColType:decimal(11,0),_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)#01-16   3.29s ± 3%   0.01s ± 6%  -99.71%  (p=0.000 n=10+10)
WindowFunctionsWithSlidingWindow/(func:count,_aggColType:int(11),_numFunc:1,_ndv:100,_rows:100000,_sorted:true,_concurrency:1)#01-16       712ms ± 2%     8ms ± 6%  -98.94%  (p=0.000 n=10+10)

note: windowing_use_high_precision=true

new.txt
old.txt

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Feb 21, 2020
@mmyj mmyj closed this Feb 21, 2020
@mmyj mmyj reopened this Feb 21, 2020
@mmyj mmyj closed this Feb 21, 2020
@mmyj mmyj reopened this Feb 23, 2020
@mmyj
Copy link
Member Author

mmyj commented Feb 23, 2020

/run-all-tests

@codecov
Copy link

codecov bot commented Feb 23, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@3906d5a). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14887   +/-   ##
===========================================
  Coverage          ?   80.4706%           
===========================================
  Files             ?        503           
  Lines             ?     133609           
  Branches          ?          0           
===========================================
  Hits              ?     107516           
  Misses            ?      17681           
  Partials          ?       8412

@mmyj
Copy link
Member Author

mmyj commented Feb 23, 2020

/run-all-tests

@SunRunAway SunRunAway self-requested a review March 1, 2020 08:49
@mmyj mmyj requested a review from alivxxx March 1, 2020 15:03
@mmyj
Copy link
Member Author

mmyj commented Mar 4, 2020

PTAL @lamxTyler @SunRunAway

SunRunAway
SunRunAway reviewed Mar 4, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

Hi, @mmyj
I'm appreciating your great job. I've left some comments, PTAL.
And it's better to change your PR title in a proper summary.

executor/aggfuncs/func_sum.go Outdated Show resolved Hide resolved
executor/aggfuncs/func_sum.go Outdated Show resolved Hide resolved
executor/aggfuncs/func_sum.go Outdated Show resolved Hide resolved
executor/aggfuncs/func_sum.go Outdated Show resolved Hide resolved
@mmyj mmyj changed the title executor: Improve the performance of WindowExec by using sliding window #14294 executor: Improve the performance of aggFuncSum by using sliding window #14294 Mar 5, 2020
@mmyj mmyj changed the title executor: Improve the performance of aggFuncSum by using sliding window #14294 executor: Improve the performance of WindowExec by using sliding window Mar 5, 2020
@mmyj mmyj requested a review from SunRunAway March 5, 2020 04:50
@mmyj mmyj changed the title executor: Improve the performance of WindowExec by using sliding window executor: Improve the performance of aggFuncSum by using sliding window Mar 5, 2020
@mmyj
Copy link
Member Author

mmyj commented Mar 12, 2020

PTAL @lamxTyler @qw4990 @wshwsh12

@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

@lamxTyler, @SunRunAway, @qw4990, @wshwsh12, PTAL.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Mar 15, 2020

@lamxTyler, @SunRunAway, @qw4990, @wshwsh12, PTAL.

@sre-bot
Copy link
Contributor

sre-bot commented Mar 18, 2020

@lamxTyler, @SunRunAway, @qw4990, @wshwsh12, PTAL.

2 similar comments
@sre-bot
Copy link
Contributor

sre-bot commented Mar 20, 2020

@lamxTyler, @SunRunAway, @qw4990, @wshwsh12, PTAL.

@sre-bot
Copy link
Contributor

sre-bot commented Mar 22, 2020

@lamxTyler, @SunRunAway, @qw4990, @wshwsh12, PTAL.

qw4990
qw4990 approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 23, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 23, 2020

@mmyj merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Mar 23, 2020

/run-all-tests

@qw4990 qw4990 merged commit bbac2b2 into pingcap:master Mar 23, 2020
@mmyj mmyj mentioned this pull request Mar 23, 2020
Merged
@you06 you06 added this to the v4.0.0-rc milestone Mar 27, 2020
This was referenced Apr 1, 2020
Merged
Merged
@mmyj mmyj mentioned this pull request Apr 6, 2020
Merged
@sre-bot sre-bot mentioned this pull request Apr 7, 2020
Merged
@mmyj mmyj mentioned this pull request Apr 25, 2020
Merged
@dveeden dveeden mentioned this pull request Jun 3, 2024
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@qw4990 qw4990 qw4990 approved these changes

@alivxxx alivxxx Awaiting requested review from alivxxx

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees
No one assigned
Labels
component/session contribution This PR is from a community contributor. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
v4.0.0-rc
Development

Successfully merging this pull request may close these issues.
7 participants
@mmyj @SunRunAway @sre-bot @qw4990 @alivxxx @zz-jason @you06