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.

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

cop: RPN function IN() #5016

Merged
merged 5 commits into from Jul 5, 2019

Conversation

Projects
None yet
3 participants
@breeswish
Copy link
Member

commented Jul 4, 2019

What have you changed? (mandatory)

This PR implements RPN function IN().

What are the type of the changes? (mandatory)

  • Improvement (change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

New unit tests.

Implement IN()
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:__batch/in branch from c3cb6fc to 87e4bb2 Jul 4, 2019

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

/run-integration-tests

@@ -207,6 +207,34 @@ pub fn coalesce<T: Evaluable>(args: &[&Option<T>]) -> Result<Option<T>> {
Ok(None)
}

#[rpn_fn(varg)]
#[inline]
pub fn compare_in<T: Evaluable + Eq>(args: &[&Option<T>]) -> Result<Option<Int>> {

This comment has been minimized.

Copy link
@sticnarf

sticnarf Jul 4, 2019

Contributor

Can we add Eq to the bound of Evaluable?

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 4, 2019

Author Member

Too many other bounds to add if we add one... For example, there are also PartialEq, PartialOrd, etc..

@sticnarf
Copy link
Contributor

left a comment

LGTM. Some optional advice is given.

BTW, when RPN function validator is supported, L213~215 can be removed or should panic. Now I can accept that the test does not cover the invalid case but maybe better to add a TODO.

Address comments
Signed-off-by: Breezewish <breezewish@pingcap.com>
Address comments again
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

@lonng @H-ZeX PTAL, thanks

@lonng

lonng approved these changes Jul 5, 2019

@lonng lonng merged commit c1223f2 into tikv:master Jul 5, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:__batch/in branch Jul 5, 2019

breeswish added a commit to breeswish/tikv that referenced this pull request Jul 9, 2019

cop: RPN function IN() (tikv#5016)
Signed-off-by: Breezewish <breezewish@pingcap.com>

breeswish added a commit to breeswish/tikv that referenced this pull request Jul 9, 2019

cop: RPN function IN() (tikv#5016)
Signed-off-by: Breezewish <breezewish@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.