-
Notifications
You must be signed in to change notification settings - Fork 2.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
Adds BatchSelectionExecutor #4562
Conversation
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
/rebuild |
rows_len, | ||
self.src.schema(), | ||
&mut src_result.data, | ||
head_retain_map.as_mut_slice(), |
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.
It seems we needn't evaluate the left conditions once we meet a false in a row/
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.
There is a todo for this. More specifically, currently we cannot partially evaluate a column (i.e. only evaluates the cell which remains true in previous filter rounds).
// The selection executor should always return empty rows. | ||
|
||
let r = exec.next_batch(1); | ||
assert_eq!(r.data.rows_len(), 0); |
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.
Could we use a loop to avoid the repeated code?
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.
it only repeats for the first 2 call. The third call is different (is_drained == true
).
|
||
// The selection executor should return data as it is. | ||
|
||
let r = exec.next_batch(1); |
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.
Maybe compare with which src_exec.next_batch
returns will be 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.
I wanted to do this, but the result is not cloneable, which makes it impossible to compare.
/// modification. | ||
/// | ||
/// Normally this should be only used in tests. | ||
pub struct MockExecutor { |
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.
Could we move it to the mod test
?
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.
I think the name "MockExecutor" already clearly indicates that it is a test facility so that it doesn't need to be "test::MockExecutor".
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
/run-integration-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.
LGTM
Signed-off-by: Breezewish <breezewish@pingcap.com>
831cef8
to
0831527
Compare
/run-integration-tests |
/rebuild |
/rebuild |
Signed-off-by: Breezewish <breezewish@pingcap.com>
6089047
to
ab8e11b
Compare
/rebuild |
/run-integration-tests |
1 similar comment
/run-integration-tests |
/rebuild |
/run-integration-tests |
/test |
/run-all-tests |
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
What have you changed? (mandatory)
This PR adds BatchSelectionExecutor.
Extracted from #3898
What are the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
Unit test