-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
executor: control Chunk size for TopN&Sort #9364
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9364 +/- ##
==========================================
+ Coverage 67.13% 67.17% +0.04%
==========================================
Files 373 373
Lines 78136 78135 -1
==========================================
+ Hits 52455 52487 +32
+ Misses 20982 20954 -28
+ Partials 4699 4694 -5
Continue to review full report at Codecov.
|
/run-all-tests |
/run-all-tests |
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.
In SortExec.fetchRowChunks()
, should we set the required rows?
Now the choice 1 is used as before. |
b6d145f
to
cc83665
Compare
maxChunkSize is better
Yuanjia Zhang <notifications@github.com>于2019年2月20日 周三14:47写道:
… In SortExec.fetchRowChunks(), should we set the required rows?
Sort is a pipeline breaker and fetchRowChunks() is used to fetch all data
from its child to sort.
For the value of requiredRows in this function, I think we have two
choices:
1. MaxChunkSize: it indicates the child return as much data as
possible once.
2. 1: it means the child should return as quickly as possible if it
has any data.
How do you think? @zz-jason <https://github.com/zz-jason>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9364 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFBlG3xz6mWyY4oHPklvUc44XmbA8iXMks5vPO98gaJpZM4bEVUR>
.
|
|
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
PTAL~ @lysu |
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
/run-all-tests |
1 similar comment
/run-all-tests |
What problem does this PR solve?
Control the number of rows in chunks returned by
LimitExec
.Following up #9354, this PR is a subtask of #9166.
What is changed and how it works?
TopN
andSort
to make them support chunk size control.Check List
Tests