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, dag: add more math functions. #2213

Merged
merged 29 commits into from Aug 29, 2017

Conversation

Projects
None yet
3 participants
@hicqu
Contributor

hicqu commented Aug 24, 2017

No description provided.

hicqu and others added some commits Aug 24, 2017

hicqu added some commits Aug 28, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Aug 28, 2017

Contributor

@AndreMouche @BusyJay , PTAL, thanks.

Contributor

hicqu commented Aug 28, 2017

@AndreMouche @BusyJay , PTAL, thanks.

}
/// ceil the Decimal into a new Decimal.
pub fn ceil(&self) -> Res<Decimal> {

This comment has been minimized.

@BusyJay

BusyJay Aug 28, 2017

Contributor

Any test to cover this?

@BusyJay

BusyJay Aug 28, 2017

Contributor

Any test to cover this?

This comment has been minimized.

@hicqu

hicqu Aug 28, 2017

Contributor

Yes, but they are in coprocessor/dag/expr/math.rs.

@hicqu

hicqu Aug 28, 2017

Contributor

Yes, but they are in coprocessor/dag/expr/math.rs.

This comment has been minimized.

@BusyJay

BusyJay Aug 28, 2017

Contributor

They are different. The unit test for this function should focus on its functionality, while tests in math.rs is used to validate if the eval is implemented correctly.

@BusyJay

BusyJay Aug 28, 2017

Contributor

They are different. The unit test for this function should focus on its functionality, while tests in math.rs is used to validate if the eval is implemented correctly.

This comment has been minimized.

@hicqu

hicqu Aug 28, 2017

Contributor

fixed.

@hicqu

hicqu Aug 28, 2017

Contributor

fixed.

("18446744073709551616", "18446744073709551616"),
("-18446744073709551615", "-18446744073709551615"),
("-18446744073709551616", "-18446744073709551616"),
("-1", "-1"),

This comment has been minimized.

@BusyJay

BusyJay Aug 28, 2017

Contributor

How about 1.000?

@BusyJay

BusyJay Aug 28, 2017

Contributor

How about 1.000?

This comment has been minimized.

@hicqu

hicqu Aug 28, 2017

Contributor

fixed.

@hicqu

hicqu Aug 28, 2017

Contributor

fixed.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Aug 28, 2017

Contributor

@BusyJay , I fixed. PTAL again, thanks.

Contributor

hicqu commented Aug 28, 2017

@BusyJay , I fixed. PTAL again, thanks.

AndreMouche and others added some commits Aug 29, 2017

Show outdated Hide outdated src/coprocessor/codec/mysql/decimal.rs
Show outdated Hide outdated src/coprocessor/dag/expr/math.rs
#[inline]
pub fn abs_int(&self, ctx: &StatementContext, row: &[Datum]) -> Result<Option<i64>> {
let n = try_opt!(self.children[0].eval_int(ctx, row));

This comment has been minimized.

@BusyJay

BusyJay Aug 29, 2017

Contributor

Can it contain unsigned flag?

@BusyJay

BusyJay Aug 29, 2017

Contributor

Can it contain unsigned flag?

This comment has been minimized.

@hicqu

hicqu Aug 29, 2017

Contributor

If it contains unsigned flag, TiDB will push down AbsUintSig.

@hicqu

hicqu Aug 29, 2017

Contributor

If it contains unsigned flag, TiDB will push down AbsUintSig.

Show outdated Hide outdated src/coprocessor/dag/expr/math.rs
Show outdated Hide outdated src/coprocessor/dag/expr/math.rs
Show outdated Hide outdated src/coprocessor/dag/expr/math.rs
Show outdated Hide outdated src/coprocessor/dag/expr/math.rs
Show outdated Hide outdated src/coprocessor/dag/expr/mod.rs
let exp = exp.eval_real(&ctx, &[]).unwrap();
assert_eq!(got, exp);
}
ScalarFuncSig::CeilIntToInt | ScalarFuncSig::CeilDecToInt => {

This comment has been minimized.

@hicqu

hicqu Aug 29, 2017

Contributor

Here I keep old code because op.eval_int(...) returns an i64 with sign. If we put it into a Datum, We can't compare it with another Datum::U64(number). @BusyJay

@hicqu

hicqu Aug 29, 2017

Contributor

Here I keep old code because op.eval_int(...) returns an i64 with sign. If we put it into a Datum, We can't compare it with another Datum::U64(number). @BusyJay

hicqu added some commits Aug 29, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Aug 29, 2017

Contributor

@BusyJay , PTAL, thanks.

Contributor

hicqu commented Aug 29, 2017

@BusyJay , PTAL, thanks.

hicqu added some commits Aug 29, 2017

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Aug 29, 2017

Contributor

LGTM

Contributor

BusyJay commented Aug 29, 2017

LGTM

hicqu added some commits Aug 29, 2017

@AndreMouche

LGTM

@AndreMouche AndreMouche merged commit 41d612e into master Aug 29, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@AndreMouche AndreMouche deleted the qupeng/expr-math branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment