-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Clean the code in limit.rs
.
#4391
Conversation
cc @jackwener |
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.
make sense to me❤️.
This PR merge
-fetch
and current_fetched
-skip
and current_skipped
Nice clean job.
/// A Limit stream skips `skip` rows, and then fetch up to `fetch` rows. | ||
struct LimitStream { | ||
/// The number of rows to skip | ||
/// The remaining number of rows to skip | ||
skip: usize, |
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.
The meaning ofskip
and fetch
has changed, we should rename them.
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.
Oh, I see this was not yet done -- @HaoYang670 can you please do this as a follow on PR? If you don't have time I will do so myself
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.
Sure, I'll file a follow-up.
Poll::Ready(Some(Ok(batch))) => { | ||
if batch.num_rows() > 0 && self.skip == 0 { | ||
break poll; | ||
} else { | ||
// continue to poll input stream | ||
} |
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.
more clear 👍
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.
Thanks @HaoYang670
Benchmark runs are scheduled for baseline = a31b44e and contender = 52e198e. 52e198e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Signed-off-by: remzi 13716567376yh@gmail.com
Which issue does this PR close?
None.
Rationale for this change
What changes are included in this PR?
current_skip
andcurrent_fetch
fromLimitStream
because they are redundant.truncate_batch
and merge its functionality intostream_limit
. Becausetruncate_batch
is not performant whenrow_num == 0
and it's only used instream_limit
Are these changes tested?
Current tests cover the changes.
Are there any user-facing changes?
No.