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: add abs rpn function #4982

Merged
merged 12 commits into from Jul 4, 2019

Conversation

Projects
None yet
7 participants
@mapleFU
Copy link
Contributor

commented Jun 27, 2019

What have you changed? (mandatory)

Add implementation and tests for rpn function abs_int, abs_uint, abs_real, abs_decimal

If you want to see what was changed, you should go for src/coprocessor/dag/rpn_expr

  • mod.rs
  • impl_math.rs

Was changed or created.

What are the type of the changes? (mandatory)

  • New feature (change which adds functionality)

How has this PR been tested? (mandatory)

New unit tests.

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

fn test_abs_real() {
let test_cases: Vec<(ScalarFuncSig, Real, Option<Real>)> = vec![
(
ScalarFuncSig::AbsReal,

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

function signature is the same, no need to leave it here.

@@ -170,6 +170,10 @@ fn map_pb_sig_to_rpn_func(value: ScalarFuncSig, children: &[Expr]) -> Result<Rpn
ScalarFuncSig::IfNullTime => if_null_fn_meta::<DateTime>(),
ScalarFuncSig::IfNullDuration => if_null_fn_meta::<Duration>(),
ScalarFuncSig::IfNullJson => if_null_fn_meta::<Json>(),
ScalarFuncSig::AbsInt => abs_int_fn_meta(),

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

Unnecessary whitespace after =>

#[rpn_fn]
#[inline]
fn abs_uint(arg: &Option<Int>) -> Result<Option<Int>> {
// TODO: make clear if it's ok

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

What's this TODO?

@mapleFU mapleFU force-pushed the mapleFU:dev-rpn-expr-abs branch from f3bfe00 to 35d8c8f Jun 27, 2019

@@ -313,6 +313,55 @@ impl ArithmeticOp for DecimalMultiply {
}
}

#[rpn_fn]
#[inline]
fn abs_int(arg: &Option<Int>) -> Result<Option<Int>> {

This comment has been minimized.

Copy link
@rleungx

rleungx Jun 27, 2019

Member

Do we need to move these functions into impl_math.rs?
/cc @breeswish

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

Nice catch. Please move it to impl_math since it is in builtin_math in TiDB.

This comment has been minimized.

Copy link
@mapleFU

mapleFU Jun 27, 2019

Author Contributor

Do we need to move these functions into impl_math.rs?
/cc @breeswish

Done

// abs returns NAN if the number is NAN, so don't worry about it
match arg {
Some(arg) => {
let f = arg.abs();

This comment has been minimized.

Copy link
@breeswish
match arg {
Some(arg) => match arg.to_owned().abs() {
Res::Ok(v) => Ok(Some(v)),
Res::Truncated(_) => Err(Error::truncated())?,

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

Decimal's Result implements Into so that Ok becomes Ok and Truncated / Overflow becomes Error. I think we can utilize this trait here.

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 4, 2019

Member

You may refer to the original implementation of abs_decimal. It is not that complex.

fn test_abs_real() {
let test_cases: Vec<(ScalarFuncSig, Real, Option<Real>)> = vec![
(
ScalarFuncSig::AbsReal,

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 27, 2019

Member

These function signatures are same. You can put it directly in .evaluate(here)

@breeswish
Copy link
Member

left a comment

The rest looks fine!

@breeswish

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Also please correctly sign-off your commits.

@breeswish breeswish added the C: Copr label Jun 28, 2019

#[rpn_fn]
#[inline]
fn abs_uint(arg: &Option<Int>) -> Result<Option<Int>> {
match arg {

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 1, 2019

Member

Simply Ok(*arg) is enough?

@breeswish breeswish added the S: Waiting label Jul 1, 2019

@mapleFU mapleFU force-pushed the mapleFU:dev-rpn-expr-abs branch 9 times, most recently from fe76295 to 2cf6ac5 Jul 1, 2019

tikv/coprocessor: And tests and implementation for AbsInt AbsUInt Abs…
…Real AbsDecimal

Support AbsInt AbsUInt AbsReal AbsDecimal

Signed-off-by: mapleFU <1506118561@qq.com>

@mapleFU mapleFU force-pushed the mapleFU:dev-rpn-expr-abs branch 4 times, most recently from 624ebce to 16b86fc Jul 1, 2019

tikv/coprocessor: Use git 'diff' and 'rebase' to restruct the commits
Signed-off-by: mapleFU <1506118561@qq.com>
@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Also sign-off is missing for your latest commit 😅

[Update] tikv/coprocessor: Remove unnecessary assert message in tests…
… for abs rpn functions in impl_math.rs

Signed-off-by: mapleFU <1506118561@qq.com>

@mapleFU mapleFU force-pushed the mapleFU:dev-rpn-expr-abs branch from a9841b6 to 9f3b1d2 Jul 4, 2019

(
ScalarFuncSig::AbsUInt,
std::u64::MAX as i64,
Some(std::u64::MAX as i64),

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Jul 4, 2019

Member

How about the case which will cause overflow or truncated?

@mapleFU mapleFU changed the title cop: add abs rpn function [WIP] cop: add abs rpn function Jul 4, 2019

[Update] coprocessor/tikv: Using num_traits::sign::Signed to simplify…
… the code for abs_real in impl_math.rs,

Signed-off-by: mapleFU <1506118561@qq.com>
use num_traits::sign::Signed;

match arg {
Some(arg) => Ok(Some(<Real as Signed>::abs(arg))),

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 4, 2019

Member

Simply:

fn abs_real(arg: &Option<Real>) -> Result<Option<Real>> {
    match arg {
        Some(arg) => Ok(Some(num_traits::Signed::abs(arg))),
        None => Ok(None),
    }
}
[Update] tikv/coprocessor: int abs_real omit use num_traits::sign::Si…
…gn and explict call it.

[Update] tikv/coprocessor: int abs_decimal using Res.into to simplify the code.

Signed-off-by: mapleFU <1506118561@qq.com>
@breeswish
Copy link
Member

left a comment

Great job! Looks good to me. @sticnarf @lonng @H-ZeX PTAL

@@ -26,25 +26,20 @@ fn abs_uint(arg: &Option<Int>) -> Result<Option<Int>> {
#[inline]
fn abs_real(arg: &Option<Real>) -> Result<Option<Real>> {
// abs returns NAN if the number is NAN, so don't worry about it

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 4, 2019

Member

Real would never be NAN. So this comment can be removed.

@breeswish breeswish added S: LGT1 and removed S: Waiting labels Jul 4, 2019

@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

/run-integration-tests

@breeswish breeswish changed the title [WIP] cop: add abs rpn function cop: add abs rpn function Jul 4, 2019

@sticnarf

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@mapleFU Add signed int overflow test case please.

[ADD] tikv/coprocessor/dag/rpn_expr: add test for Int Overflow in tes…
…t_abs_int

Signed-off-by: mapleFU <1506118561@qq.com>
[ADD] tikv/coprocessor/dag/rpn_expr: patch format for last commit
Signed-off-by: mapleFU <1506118561@qq.com>
@sticnarf
Copy link
Contributor

left a comment

Looks awesome

@@ -25,7 +25,7 @@ fn abs_uint(arg: &Option<Int>) -> Result<Option<Int>> {
#[rpn_fn]
#[inline]
fn abs_real(arg: &Option<Real>) -> Result<Option<Real>> {
// abs returns NAN if the number is NAN, so don't worry about it
// arg is never nan

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 4, 2019

Member

So why not simply remove this comment 🤣 This comment provides no information, which looks to be useless.

[remove] tikv/coprocessor/dag/rpn_expr: remove annotation in abs_real
Signed-off-by: mapleFU <1506118561@qq.com>

@breeswish breeswish merged commit 41086a9 into tikv:master Jul 4, 2019

2 checks passed

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

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

cop: add rpn function ABS (tikv#4982)
Signed-off-by: mapleFU <1506118561@qq.com>

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

cop: add rpn function ABS (tikv#4982)
Signed-off-by: MWish <1506118561@qq.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.