coprocessor, dag: add more math functions. #2213

hicqu · 2017-08-24T07:32:52Z

No description provided.

BusyJay · 2017-08-24T07:36:18Z

src/coprocessor/codec/mysql/decimal.rs

@@ -835,6 +838,42 @@ pub enum RoundMode {
 }

 impl Decimal {
+    /// abs the Decimal into a new Decimal.
+    #[inline]
+    pub fn abs(&self) -> Res<Decimal> {


Let's move it instead.

BusyJay · 2017-08-24T07:37:06Z

src/coprocessor/codec/mysql/decimal.rs

@@ -1428,6 +1466,35 @@ impl Decimal {
        Res::Ok(x)
    }

+    /// `int_part` returns int part of the decimal. It's temporary and
+    /// after we adjust `as_i64`, it will be removed.
+    pub fn int_part(&self, ctx: &EvalContext) -> ::std::result::Result<i64, ExprError> {


I don't get it, what's wrong with as_i64?

as_i64 doesn't retrieve StatementContext argument, so that if any Truncate occurs, it can only return it by Res::Truncate. It causes that we need to test that everywhere (e.g. ceil/floor and cast). We need to write code everywhere like this:

fn ceil_dec_to_int(...) { match dec.as_i64() { Res::Ok(t) => t, Truncate(t) => None, // something here. _ => None, // some other thing. } }

So I think we can pass Statement into as_i64. If any truncate occurs inside that function, we just need to check the flag, and return Ok(t) or Err directly.

I think you can write a function to deal with both Res and truncate strategy instead.

So the old behavior doesn't have to change.

BusyJay · 2017-08-24T07:39:02Z

src/coprocessor/dag/expr/mod.rs

@@ -228,6 +230,13 @@ impl Expression {
                ScalarFuncSig::TimeIsNull => f.time_is_null(ctx, row),
                ScalarFuncSig::DurationIsNull => f.duration_is_null(ctx, row),

+                ScalarFuncSig::AbsInt => f.abs_int(ctx, row),
+                ScalarFuncSig::AbsUInt => f.abs_uint(ctx, row),


Why not handle unsigned int and signed int together just like plus?

TiDB pushdown both AbsInt and AbsUint. So here I follow that.

Can we always merge int and uint or split them? /cc @XuHuaiyu

BusyJay · 2017-08-24T07:52:09Z

src/coprocessor/codec/mysql/decimal.rs

+
+    /// ceil the Decimal into a new Decimal.
+    pub fn ceil(&self) -> Res<Decimal> {
+        let mut target = if self.frac_cnt > 0 && !self.negative {


Why not use t.round(0, RoundMode::Ceiling) directly?

because ceil(-5.5) should return -5, but round(-5.5, RoundMode::Ceiling) will return -6.

Can we fix the bug in t.round(0, RoundMode::Ceiling) directly?

Can we fix the bug in t.round(0, RoundMode::Ceiling) directly? @hicqu

round is used many place, and it's different from math.Ceil semantically. I prefer use different RoundMode name, such as RoundUp and RoundDown. They are more clearly. If it's OK, I will do it in next PR.

BusyJay · 2017-08-24T07:52:36Z

src/coprocessor/codec/mysql/decimal.rs

+
+    /// floor the Decimal into a new Decimal.
+    pub fn floor(&self) -> Res<Decimal> {
+        let mut target = if self.frac_cnt > 0 && self.negative {


Why not use t.round(0, RoundMode::Truncate)` directly?

because floor(-5.5) should return -6, but that will return -5.

AndreMouche · 2017-08-28T02:25:02Z

src/coprocessor/codec/mysql/decimal.rs

+
+    /// ceil the Decimal into a new Decimal.
+    pub fn ceil(&self) -> Res<Decimal> {
+        let mut target = if self.frac_cnt > 0 && !self.negative {


Can we fix the bug in t.round(0, RoundMode::Ceiling) directly? @hicqu

AndreMouche · 2017-08-28T02:49:25Z

src/coprocessor/codec/mysql/decimal.rs

+    pub fn as_i64_with_ctx(&self, ctx: &EvalContext) -> ::std::result::Result<i64, ExprError> {
+        let res = self.as_i64();
+        if ctx.ignore_truncate || ctx.truncate_as_warning {
+            if let Res::Truncated(ref i) = res {


Could we use convert::handle_truncate here since one day we may need to process truncate as warning?

AndreMouche · 2017-08-28T02:54:44Z

src/coprocessor/dag/expr/math.rs

+    }
+
+    #[inline]
+    pub fn ceil_int_dec<'a, 'b: 'a>(


ceil_int_to_dec/ceil_int_as_dec?

hicqu · 2017-08-28T06:55:41Z

@AndreMouche @BusyJay , PTAL, thanks.

BusyJay · 2017-08-28T07:18:22Z

src/coprocessor/codec/mysql/decimal.rs

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


Any test to cover this?

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

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.

…g/expr-math

BusyJay · 2017-08-28T08:14:52Z

src/coprocessor/codec/mysql/decimal.rs

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


How about 1.000?

…g/expr-math

hicqu · 2017-08-28T11:31:35Z

@BusyJay , I fixed. PTAL again, thanks.

…g/expr-math

BusyJay · 2017-08-28T12:13:45Z

src/coprocessor/codec/mysql/decimal.rs

+
+    /// ceil the Decimal into a new Decimal.
+    pub fn ceil(&self) -> Res<Decimal> {
+        let mut target = if !self.frac_part_is_zero() && !self.negative {


How about use t.round(0, RoundMode::Ceiling) when it's positive otherwise use t.round(0, RoundMode::Truncate)?

BusyJay · 2017-08-28T12:29:09Z

src/coprocessor/codec/mysql/decimal.rs

+
+    /// floor the Decimal into a new Decimal.
+    pub fn floor(&self) -> Res<Decimal> {
+        let mut target = if !self.frac_part_is_zero() && self.negative {


How about use t.round(0, RoundMode::Truncate) when it's positive otherwise use t.round(0, RoundMode::Ceiling)?

BusyJay · 2017-08-28T12:32:34Z

src/coprocessor/codec/mysql/decimal.rs

+    pub fn floor(&self) -> Res<Decimal> {
+        let mut target = if !self.frac_part_is_zero() && self.negative {
+            let dec1 = Decimal::from(1i64);
+            self - &dec1


Will it overflow/truncate if self is 999999.999999(enough 9)?

BusyJay · 2017-08-29T04:50:23Z

src/coprocessor/codec/mysql/decimal.rs

+                "10000000000000000000000000",
+            ),
+        ];
+        for case in cases {


for (input, exp) in cases is more readable.

BusyJay · 2017-08-29T04:54:33Z

src/coprocessor/dag/expr/math.rs

+        row: &'a [Datum],
+    ) -> Result<Option<Cow<'a, Decimal>>> {
+        let d = try_opt!(self.children[0].eval_decimal(ctx, row));
+        let result: Result<Decimal> = d.as_ref().clone().abs().into();


Why not use into_owned?

BusyJay · 2017-08-29T04:56:12Z

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));


Can it contain unsigned flag?

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

BusyJay · 2017-08-29T04:58:51Z

src/coprocessor/dag/expr/math.rs

+
+    #[inline]
+    pub fn ceil_int_to_int(&self, ctx: &StatementContext, row: &[Datum]) -> Result<Option<i64>> {
+        self.children[0].eval_int(ctx, row)


So what's the purpose of this function?

When parsing SQL, TiDB won't remove ceil operator on int, it will calculate that. So it also push CeilIntIntSig down to TiKV. Although we know it does nothing, but we should deal with it.

Can optimizer optimize this? /cc @hanfei1991

BusyJay · 2017-08-29T04:59:08Z

src/coprocessor/dag/expr/math.rs

+        ctx: &StatementContext,
+        row: &'a [Datum],
+    ) -> Result<Option<Cow<'a, Decimal>>> {
+        let n = try_opt!(self.children[0].eval_int(ctx, row));


Why not just use cast_int_as_dec?

BusyJay · 2017-08-29T05:01:43Z

src/coprocessor/dag/expr/math.rs

+    }
+
+    #[inline]
+    pub fn floor_int_dec<'a, 'b: 'a>(


s/floor_int_dec/floor_int_to_dec/

BusyJay · 2017-08-29T05:04:15Z

src/coprocessor/dag/expr/math.rs

+        for tt in tests {
+            let arg = datum_expr(tt.1);
+            let op = Expression::build(fncall_expr(tt.0, &[arg]), 0, &ctx).unwrap();
+            let expected = Expression::build(datum_expr(tt.2), 0, &ctx).unwrap();


This is redundant. See #2215.

BusyJay · 2017-08-29T05:05:16Z

src/coprocessor/dag/expr/mod.rs

@@ -228,6 +230,13 @@ impl Expression {
                ScalarFuncSig::TimeIsNull => f.time_is_null(ctx, row),
                ScalarFuncSig::DurationIsNull => f.duration_is_null(ctx, row),

+                ScalarFuncSig::AbsInt => f.abs_int(ctx, row),
+                ScalarFuncSig::AbsUInt => f.abs_uint(ctx, row),


Can we always merge int and uint or split them? /cc @XuHuaiyu

hicqu · 2017-08-29T06:13:50Z

src/coprocessor/dag/expr/math.rs

+                    let exp = exp.eval_real(&ctx, &[]).unwrap();
+                    assert_eq!(got, exp);
+                }
+                ScalarFuncSig::CeilIntToInt | ScalarFuncSig::CeilDecToInt => {


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 · 2017-08-29T07:49:28Z

@BusyJay , PTAL, thanks.

AndreMouche · 2017-08-29T08:59:21Z

src/coprocessor/dag/expr/math.rs

+        ctx: &StatementContext,
+        row: &'a [Datum],
+    ) -> Result<Option<Cow<'a, Decimal>>> {
+        self.cast_int_as_decimal(ctx, row)


I think tidb should push down CastIntAsDec instead of CeilIntToDec to increase match's efficiency @XuHuaiyu

BusyJay · 2017-08-29T09:43:07Z

LGTM

AndreMouche

LGTM

coprocessor, dag: add more math functions.

44a8cd5

BusyJay reviewed Aug 24, 2017

View reviewed changes

hicqu and others added 2 commits August 24, 2017 19:24

address comments.

13c77ed

Merge branch 'master' into qupeng/expr-math

6ad8c1a

AndreMouche reviewed Aug 28, 2017

View reviewed changes

hicqu added 5 commits August 28, 2017 13:11

Merge branch 'master' into qupeng/expr-math

4f298f0

address comments.

9d5a996

fix CI.

1451b9d

fix CI.

8ae0825

Merge branch 'master' into qupeng/expr-math

8a3992d

BusyJay reviewed Aug 28, 2017

View reviewed changes

hicqu added 4 commits August 28, 2017 16:00

address comments.

0d30268

Merge branch 'qupeng/expr-math' of github.com:pingcap/tikv into qupen…

c99aaa1

…g/expr-math

format code for CI.

6d82c95

Merge branch 'master' into qupeng/expr-math

0d1827d

BusyJay reviewed Aug 28, 2017

View reviewed changes

hicqu added 3 commits August 28, 2017 19:24

address comments.

9bcbb9f

Merge branch 'qupeng/expr-math' of github.com:pingcap/tikv into qupen…

e254363

…g/expr-math

Merge branch 'master' into qupeng/expr-math

a9bbed2

hicqu added 2 commits August 28, 2017 19:32

fix CI.

a7c6b78

Merge branch 'qupeng/expr-math' of github.com:pingcap/tikv into qupen…

e4c1939

…g/expr-math

BusyJay reviewed Aug 28, 2017

View reviewed changes

AndreMouche and others added 3 commits August 29, 2017 10:04

Merge branch 'master' into qupeng/expr-math

ea06295

address comments.

4baf7ab

Merge branch 'master' into qupeng/expr-math

0fe793c

BusyJay reviewed Aug 29, 2017

View reviewed changes

address comments.

b78130e

hicqu commented Aug 29, 2017

View reviewed changes

hicqu added 3 commits August 29, 2017 14:17

address comments.

db84fe0

address comments.

fb33d5e

Merge branch 'master' into qupeng/expr-math

63af906

AndreMouche reviewed Aug 29, 2017

View reviewed changes

hicqu added 3 commits August 29, 2017 17:21

clean code.

d641b87

make CI happy.

0e7d2b2

address comments.

5cc2ad9

hicqu added 2 commits August 29, 2017 18:16

make ci happy.

7a9430d

address comments.

c52ed51

AndreMouche approved these changes Aug 29, 2017

View reviewed changes

AndreMouche merged commit 41d612e into master Aug 29, 2017

AndreMouche deleted the qupeng/expr-math branch August 29, 2017 13:37

coprocessor, dag: add more math functions. #2213

coprocessor, dag: add more math functions. #2213

Conversation

hicqu commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Aug 29, 2017

Choose a reason for hiding this comment

BusyJay commented Aug 29, 2017

AndreMouche left a comment

Choose a reason for hiding this comment