tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB #5468

H-ZeX · 2019-09-16T14:58:21Z

Signed-off-by: H-ZeX hzx20112012@gmail.com

merge check and fix inconsistency between TiKV and TiDB's cast (part 1) #5423 first. After merge check and fix inconsistency between TiKV and TiDB's cast (part 1) #5423, the most of this PR is unit test, so it is easy to review, althouth the changed line number may seems very large.

What have you changed?

compare tidb and tikv's impl of cast and fix what they are inconsistent.

What is the type of the changes?

Improvement (a change which is an improvement to an existing feature)
Bugfix (a change which fixes an issue)
Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

unit test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No.

Does this PR affect `tidb-ansible`?

No.

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

…iff_part_1

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

sre-bot · 2019-09-16T14:58:28Z

Thanks for your PR.
This PR will be closed by bot because you already had 3 opened PRs, close or merge them before submitting a new one.
#5423 , #5431 , #5433

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

…iff_part_5 Signed-off-by: H-ZeX <hzx20112012@gmail.com>

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

…iff_part_5 Signed-off-by: H-ZeX <hzx20112012@gmail.com>

iosmanthus · 2019-10-09T11:35:13Z

components/tidb_query/src/codec/convert.rs

+                            if let Error::Eval(_, d) = r.err().unwrap() {
+                                assert_eq!(d, ERR_M_BIGGER_THAN_D, "{}", log);
+                            } else {
+                                panic!("unreachable path, {}", log);


Should we use unreachable!() instead?

iosmanthus · 2019-10-09T11:35:38Z

components/tidb_query/src/codec/convert.rs

+                                panic!("unreachable path, {}", log);
+                            }
+                        }
+                        _ => panic!("unreachable path, {}", log),


iosmanthus · 2019-10-09T11:37:13Z

components/tidb_query/src/codec/mysql/decimal.rs

@@ -1907,6 +1908,7 @@ impl ToString for Decimal {
 impl Display for Decimal {
    fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
        let mut dec = self.clone();
+        // TODO: why there is rounding here?


Please fix this TODO note, make it more declarative.

iosmanthus · 2019-10-09T11:40:28Z

components/tidb_query/src/rpn_expr/impl_cast.rs

@@ -779,6 +924,25 @@ mod tests {
        EvalContext::new(cfg)
    }

+    fn make_ctx_4(


Could we apply a builder pattern here? Create a configuration structure to describe such flags.

Different test need different flag, so if will create a config, it should had all flag as option. Then it will same as EvalContext.
This is test, and rust has no overload (if use trait, it is too troublesome). So it need to seperate these functions will _x

iosmanthus · 2019-10-09T11:42:12Z

components/tidb_query/src/rpn_expr/impl_cast.rs

+        if truncate_as_warning {
+            flag |= Flag::TRUNCATE_AS_WARNING;
+        }
+        for i in flags {


flag |= flags.fold(Flag::new(), |acc, x| acc | x) looks better?

iosmanthus · 2019-10-09T11:43:25Z

components/tidb_query/src/rpn_expr/impl_cast.rs

+        #[derive(Clone, Copy, Debug)]
+        #[allow(clippy::enum_variant_names)]
+        enum Cond {
+            TargetIntPartLenLessThanOriginIntPartLen,


Could we shorten these names?

If shorten it, then I need to add doc for it. I think add doc is not a good idea.
There is many long function name in TiKV.

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

andylokandy · 2019-10-11T06:46:17Z

components/tidb_query/src/rpn_expr/impl_cast.rs

+            let res = if in_union(extra.implicit_args) && val.is_negative() {
+                Decimal::zero()
+            } else {
+                // FIXME: here TiDB may has bug, fix this after fix TiDB's


Same as above

lonng · 2019-10-11T07:27:48Z

components/tidb_query/src/codec/mysql/decimal.rs

@@ -434,8 +434,9 @@ fn do_sub<'a>(mut lhs: &'a Decimal, mut rhs: &'a Decimal) -> Res<Decimal> {
    res
 }

+// TODO: add check for prec and frac_cnt


Prefer to remove this comment and add an assert(prec > frac_cnt) (this TODO is meaningless because it's easy to fix it).

lonng · 2019-10-11T07:30:15Z

components/tidb_query/src/rpn_expr/impl_cast.rs

+// because uint's upper bound is smaller than signed decimal's upper bound
+// so we can merge cast uint as signed/unsigned decimal in this function


I think to move the two lines comments to above L641 is appropriate.

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

lonng · 2019-10-12T01:48:48Z

components/tidb_query/src/codec/mysql/decimal.rs

+/// If `prec` > `frac_cnt`, panic.
+/// The panic is because of `debug_assert`.
+
+// TODO: what if prec-frac_cnt==0


It's OK if the int_cnt is zero.

create table td (a decimal(2,2)); insert into td values ('.99');

lonng · 2019-10-12T01:50:36Z

components/tidb_query/src/codec/mysql/decimal.rs

@@ -435,7 +435,16 @@ fn do_sub<'a>(mut lhs: &'a Decimal, mut rhs: &'a Decimal) -> Res<Decimal> {
 }

 /// Get the max possible decimal with giving precision and fraction digit count.
-fn max_decimal(prec: u8, frac_cnt: u8) -> Decimal {
+/// The `prec` should <= `frac_cnt`.


Suggested change

/// The `prec` should <= `frac_cnt`.

/// The `prec` should >= `frac_cnt`.

lonng · 2019-10-12T01:50:57Z

components/tidb_query/src/codec/mysql/decimal.rs

@@ -465,6 +474,12 @@ fn max_decimal(prec: u8, frac_cnt: u8) -> Decimal {

 /// `max_or_min_dec`(`NewMaxOrMinDec` in tidb) returns the max or min
 /// value decimal for given precision and fraction.
+/// The `prec` should <= `frac_cnt`.


Signed-off-by: H-ZeX <hzx20112012@gmail.com>

components/tidb_query/src/codec/mysql/decimal.rs

lonng · 2019-10-12T02:57:45Z

components/tidb_query/src/codec/mysql/decimal.rs

+///
+/// # Panics
+///
+/// If `prec` > `frac_cnt`, panic.


Signed-off-by: H-ZeX <hzx20112012@gmail.com>

lonng

LGTM

andylokandy

LGTM

breezewish · 2019-10-12T07:10:53Z

/merge

sre-bot · 2019-10-12T07:10:57Z

Your auto merge job has been accepted, waiting for 5635

sre-bot · 2019-10-12T07:26:05Z

/run-all-tests

sre-bot · 2019-10-12T07:38:33Z

@H-ZeX merge failed.

lonng · 2019-10-12T07:54:25Z

/test

…ikv#5468) Signed-off-by: H-ZeX <hzx20112012@gmail.com>

H-ZeX added 5 commits September 9, 2019 11:57

fix_cast_diff part_1

f2793de

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

Merge branch 'master' of https://github.com/tikv/tikv into fix_cast_d…

495a271

…iff_part_1

fix_cast_diff_part_4(* as string)

5fd511d

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

make format

06201e1

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

fix_cast_diff_part_5(* as decimal)

31d095d

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

sre-bot added the S: BotClose label Sep 16, 2019

sre-bot closed this Sep 16, 2019

H-ZeX added the sig/coprocessor SIG: Coprocessor label Sep 16, 2019

fix some bug

c49083e

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

H-ZeX reopened this Sep 24, 2019

H-ZeX added 4 commits September 24, 2019 16:41

Merge branch 'master' of https://github.com/tikv/tikv into fix_cast_d…

bf22016

…iff_part_5 Signed-off-by: H-ZeX <hzx20112012@gmail.com>

address comment

07d11df

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

address comment

860a6fb

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

address comment

60b446e

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

mapleFU removed the S: BotClose label Sep 25, 2019

This was referenced Sep 26, 2019

tidb_query: fix CAST as duration inconsistency between TiKV and TiDB #5540

Merged

tidb_query: fix some err in cast_string_as_int_or_uint #5543

Merged

tidb_query: fix CAST as json inconsistency between TiKV and TiDB #5548

Merged

lonng changed the title ~~check and fix inconsistency between TiKV and TiDB's cast (part 5)(* as decimal)~~ tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB Sep 27, 2019

H-ZeX added 2 commits September 27, 2019 13:39

address comment

777e429

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

Merge branch 'master' of https://github.com/tikv/tikv into fix_cast_d…

478a657

…iff_part_5 Signed-off-by: H-ZeX <hzx20112012@gmail.com>

iosmanthus reviewed Oct 9, 2019

View reviewed changes

iosmanthus and others added 2 commits October 9, 2019 19:43

Merge branch 'master' into fix_cast_diff_part_5

6950c80

address comment

0d87ff0

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

andylokandy reviewed Oct 11, 2019

View reviewed changes

lonng reviewed Oct 11, 2019

View reviewed changes

H-ZeX added 2 commits October 11, 2019 16:05

address comment

c636325

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

address comment

498dcae

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

H-ZeX force-pushed the fix_cast_diff_part_5 branch from f8a6c0e to 498dcae Compare October 11, 2019 10:03

H-ZeX added 2 commits October 11, 2019 18:05

address comment

f46cccc

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

fix ci failed

b1bc65a

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

lonng reviewed Oct 12, 2019

View reviewed changes

address comment

7aee840

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

lonng reviewed Oct 12, 2019

View reviewed changes

components/tidb_query/src/codec/mysql/decimal.rs Outdated Show resolved Hide resolved

lonng reviewed Oct 12, 2019

View reviewed changes

components/tidb_query/src/codec/mysql/decimal.rs Outdated

///

/// # Panics

///

/// If `prec` > `frac_cnt`, panic.

Copy link

Member

lonng Oct 12, 2019

Choose a reason for hiding this comment

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

ditto

address comment

664f7b3

Signed-off-by: H-ZeX <hzx20112012@gmail.com>

lonng approved these changes Oct 12, 2019

View reviewed changes

andylokandy approved these changes Oct 12, 2019

View reviewed changes

breezewish approved these changes Oct 12, 2019

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 12, 2019

H-ZeX and others added 2 commits October 12, 2019 15:14

Merge branch 'master' into fix_cast_diff_part_5

23a9e89

Merge branch 'master' into fix_cast_diff_part_5

bd82624

lonng merged commit 0e355bf into tikv:master Oct 12, 2019

lonng deleted the fix_cast_diff_part_5 branch October 12, 2019 08:00

aknuds1 pushed a commit to aknuds1/tikv that referenced this pull request Oct 13, 2019

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB (t…

b998698

…ikv#5468) Signed-off-by: H-ZeX <hzx20112012@gmail.com>

sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB (t…

a506476

…ikv#5468) Signed-off-by: H-ZeX <hzx20112012@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB #5468

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB #5468

H-ZeX commented Sep 16, 2019 •

edited by mapleFU

Loading

sre-bot commented Sep 16, 2019

iosmanthus Oct 9, 2019

iosmanthus Oct 9, 2019

iosmanthus Oct 9, 2019

iosmanthus Oct 9, 2019

H-ZeX Oct 9, 2019

iosmanthus Oct 9, 2019

iosmanthus Oct 9, 2019

H-ZeX Oct 9, 2019 •

edited

Loading

andylokandy Oct 11, 2019

lonng Oct 11, 2019

lonng Oct 11, 2019

lonng Oct 12, 2019 •

edited

Loading

lonng Oct 12, 2019

lonng Oct 12, 2019

lonng Oct 12, 2019

lonng left a comment

andylokandy left a comment

breezewish commented Oct 12, 2019

sre-bot commented Oct 12, 2019

sre-bot commented Oct 12, 2019

sre-bot commented Oct 12, 2019

lonng commented Oct 12, 2019

		// because uint's upper bound is smaller than signed decimal's upper bound
		// so we can merge cast uint as signed/unsigned decimal in this function

	/// The `prec` should <= `frac_cnt`.
	/// The `prec` should >= `frac_cnt`.

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB #5468

tidb_query: fix CAST as decimal inconsistency between TiKV and TiDB #5468

Conversation

H-ZeX commented Sep 16, 2019 • edited by mapleFU Loading

What have you changed?

What is the type of the changes?

How is the PR tested?

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect tidb-ansible?

sre-bot commented Sep 16, 2019

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

H-ZeX Oct 9, 2019 • edited Loading

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

lonng Oct 12, 2019 • edited Loading

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

lonng left a comment

Choose a reason for hiding this comment

andylokandy left a comment

Choose a reason for hiding this comment

breezewish commented Oct 12, 2019

sre-bot commented Oct 12, 2019

sre-bot commented Oct 12, 2019

sre-bot commented Oct 12, 2019

lonng commented Oct 12, 2019

H-ZeX commented Sep 16, 2019 •

edited by mapleFU

Loading

Does this PR affect `tidb-ansible`?

H-ZeX Oct 9, 2019 •

edited

Loading

lonng Oct 12, 2019 •

edited

Loading