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

coprocessor: add mod(Int/Real/Decimal) RPN functions #4727

Merged
merged 6 commits into from
May 21, 2019
Merged

coprocessor: add mod(Int/Real/Decimal) RPN functions #4727

merged 6 commits into from
May 21, 2019

Conversation

lonng
Copy link
Member

@lonng lonng commented May 20, 2019

Signed-off-by: Lonng heng@lonng.org

What have you changed? (mandatory)

Add mod (Int/Real/Decimal) RPN functions

What are the type of the changes? (mandatory)

  • New feature (change which adds functionality)

How has this PR been tested? (mandatory)

unit tests

@lonng lonng added the sig/coprocessor SIG: Coprocessor label May 20, 2019
@lonng
Copy link
Member Author

lonng commented May 20, 2019

/run-integration-tests

lonng added 2 commits May 21, 2019 10:29
Signed-off-by: Lonng <heng@lonng.org>
Signed-off-by: Lonng <heng@lonng.org>
@lonng
Copy link
Member Author

lonng commented May 21, 2019

@breeswish @kennytm @sticnarf PTAL

@@ -65,6 +65,15 @@ fn plus_mapper(lhs_is_unsigned: bool, rhs_is_unsigned: bool) -> Box<dyn RpnFunct
}
}

fn mod_mapper(lhs_is_unsigned: bool, rhs_is_unsigned: bool) -> Box<dyn RpnFunction> {
match (lhs_is_unsigned, rhs_is_unsigned) {
(false, false) => Box::new(RpnFnArithmetic::<IntIntMod>::new()),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can discover some higher ordered generics to deal with these IntInt, IntUint, UintInt, UintUint later, to reduce code duplicates :) We will have many functions that is different in signed / unsigned integers. Would be too verbose if we need a standalone function for each of them.

@breezewish
Copy link
Member

Cool! I will click the approve button after resolving comments above.

lonng added 2 commits May 21, 2019 13:15
Signed-off-by: Lonng <heng@lonng.org>
sticnarf
sticnarf previously approved these changes May 21, 2019
breezewish
breezewish previously approved these changes May 21, 2019
@breezewish
Copy link
Member

/run-integration-tests

@breezewish breezewish dismissed stale reviews from sticnarf and themself via 312988c May 21, 2019 06:42
@breezewish
Copy link
Member

/run-integration-tests

#[test]
fn test_mod_real() {
let tests = vec![
(Real::new(1.0).ok(), None, None),
Copy link
Contributor

@sticnarf sticnarf May 21, 2019

Choose a reason for hiding this comment

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

Maybe we can write Option<f64> here and convert it below in the loop to make the test vec clearer. (I can do it later)

@sticnarf sticnarf merged commit 617da41 into tikv:master May 21, 2019
@lonng lonng deleted the rpn-mod branch May 21, 2019 13:28
jswh pushed a commit to jswh/tikv that referenced this pull request May 27, 2019
* coprocessor: add mod(Int/Real/Decimal) RPN functions

Signed-off-by: Lonng <heng@lonng.org>

* remove empty line

Signed-off-by: Lonng <heng@lonng.org>

* address comment

Signed-off-by: Lonng <heng@lonng.org>
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
* coprocessor: add mod(Int/Real/Decimal) RPN functions

Signed-off-by: Lonng <heng@lonng.org>

* remove empty line

Signed-off-by: Lonng <heng@lonng.org>

* address comment

Signed-off-by: Lonng <heng@lonng.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/coprocessor SIG: Coprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants