expression: fix cast string like '.1a1' to decimal has no warnings information #26005

yuqi1129 · 2021-07-06T12:44:52Z

What problem does this PR solve?

Issue Number: close #26004

Problem Summary:

also fix problem like

select cast('1.4a' as decimal); 

mysql> show warnings;
+---------+------+------------------------------------------+
| Level   | Code | Message                                  |
+---------+------+------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '1.4' |
+---------+------+------------------------------------------+
1 row in set (0.02 sec)

warning information is 1.4 not 1.4a

mysql> select cast('' as decimal);
+---------------------+
| cast('' as decimal) |
+---------------------+
|                   0 |
+---------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+------------+
| Level   | Code | Message    |
+---------+------+------------+
| Warning | 8029 | Bad Number |
+---------+------+------------+
1 row in set (0.00 sec)

warning info is inconsistent with MySQL

mysql> select cast('1e - 1' as decimal)
    -> ;
+---------------------------+
| cast('1e - 1' as decimal) |
+---------------------------+
|                         1 |
+---------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+------------------------------------------+
| Level   | Code | Message                                  |
+---------+------+------------------------------------------+
| Warning | 1265 | Data truncated for column '%s' at row %d |
+---------+------+------------------------------------------+
1 row in set (0.00 sec)

Warning info is somewhat Semi-finished products

mysql> select cast('1 1' as decimal);
+------------------------+
| cast('1 1' as decimal) |
+------------------------+
|                      1 |
+------------------------+
1 row in set (0.00 sec)

The above SQL should have warning information

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Integration test

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

No release note.

… get error

…fromation

ti-chi-bot · 2021-07-06T12:44:53Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

…fromation

yuqi1129 · 2021-07-10T08:52:45Z

/cc @wshwsh12 @lzmhhh123

tisonkun

I agree on the original motivation to be compatible with MySQL. But the change set seems affect more than one behavior.

Would you mind to list the compatibility we'd like to keep preciously in the issue first? You can prepare your PR locally but we may figure out the problem first instead of rush into the code.

tisonkun · 2021-07-10T09:06:40Z

ddl/column_type_change_test.go

@@ -552,13 +552,14 @@ func (s *testColumnTypeChangeSuite) TestColumnTypeChangeFromStringToOthers(c *C)
 	tk.MustExec("insert into t values ('123.45', '123.45', '123.45', '123.45', '123.45', '123.45', '123', '123')")
 	tk.MustExec("alter table t modify c decimal(7, 4)")
 	tk.MustExec("alter table t modify vc decimal(7, 4)")
-	tk.MustExec("alter table t modify bny decimal(7, 4)")
+	// alter binary 0x3132332E34350000 to decimal(7,4) will get error
+	tk.MustGetErrCode("alter table t modify bny decimal(7, 4)", mysql.ErrTruncatedWrongValue)


We change existing test case here. Does it mean this PR changes TiDB behavior explicitly (more than a warning)?

Yes, In MySQL, this case will directly return error

tisonkun · 2021-07-10T09:06:53Z

ddl/column_type_change_test.go

 	tk.MustExec("alter table t modify vbny decimal(7, 4)")
 	tk.MustExec("alter table t modify bb decimal(7, 4)")
 	tk.MustExec("alter table t modify txt decimal(7, 4)")
 	tk.MustExec("alter table t modify e decimal(7, 4)")
 	tk.MustExec("alter table t modify s decimal(7, 4)")
-	tk.MustQuery("select * from t").Check(testkit.Rows("123.4500 123.4500 123.4500 123.4500 123.4500 123.4500 1.0000 1.0000"))
+	tk.MustQuery("select * from t").Check(testkit.Rows("123.4500 123.4500 123.45\x00\x00 123.4500 123.4500 123.4500 1.0000 1.0000"))


ditto, it seems result changes.

see the reason above

tisonkun · 2021-07-10T09:07:25Z

expression/builtin_cast_vec.go

@@ -836,7 +836,7 @@ func (b *builtinCastRealAsDecimalSig) vecEvalDecimal(input *chunk.Chunk, result
 				if types.ErrOverflow.Equal(err) {
 					warnErr := types.ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", b.args[0])
 					err = b.ctx.GetSessionVars().StmtCtx.HandleOverflow(err, warnErr)
-				} else if types.ErrTruncated.Equal(err) {
+				} else if strings.Contains(err.Error(), "Truncated incorrect") {


Why do we do string matching for identifying an error? It is quite brittle.

Agree, sorry, i can't find more elegant methods to handle this, please do me a favor about this point

Can we use types.ErrTruncatedWrongVal.Equal(err)?

Non-vec implement also need modify.

Are you sure we need modify CastRealAsDecimal, instead of CastStringAsDecimal?

Is there any related test? I didn't find the test cover the case.. Or maybe I ignore the test?

May also need to modify the implementation in tikv....

tisonkun · 2021-07-10T09:07:49Z

types/convert_test.go

@@ -244,10 +244,11 @@ func (s *testTypeConvertSuite) TestConvertType(c *C) {
 	c.Assert(terror.ErrorEqual(err, ErrOverflow), IsTrue, Commentf("err %v", err))
 	c.Assert(v.(*MyDecimal).String(), Equals, "-9999.9999")
 	v, err = Convert("1,999.00", ft)
-	c.Assert(terror.ErrorEqual(err, ErrBadNumber), IsTrue, Commentf("err %v", err))
+	c.Assert(terror.ErrorEqual(err,


ditto behavior changes

tisonkun · 2021-07-10T09:07:54Z

types/convert_test.go

 	c.Assert(v.(*MyDecimal).String(), Equals, "1.0000")
 	v, err = Convert("1,999,999.00", ft)
-	c.Assert(terror.ErrorEqual(err, ErrBadNumber), IsTrue, Commentf("err %v", err))
+	c.Assert(terror.ErrorEqual(err, ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1,999,999.00")), IsTrue, Commentf("err %v", err))


ditto behavior changes

tisonkun · 2021-07-10T09:08:22Z

types/mydecimal.go

 	if len(str) == 0 {
 		*d = zeroMyDecimal
-		return ErrBadNumber
+		return ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(strBak))


ditto behavior changes

tisonkun · 2021-07-10T09:09:24Z

types/mydecimal.go

@@ -549,6 +576,7 @@ func (d *MyDecimal) FromString(str []byte) error {
 	if strErr != nil {
 		return strErr
 	}
+


please avoid unnecessary diff.

tisonkun · 2021-07-10T09:09:58Z

types/parser_driver/value_expr.go

@@ -46,7 +46,8 @@ func init() {
 	ast.NewDecimal = func(str string) (interface{}, error) {
 		dec := new(types.MyDecimal)
 		err := dec.FromString(hack.Slice(str))
-		if err == types.ErrTruncated {
+		if err == types.ErrTruncated || (err != nil &&


ditto brittle string matching.

tisonkun · 2021-07-10T09:17:53Z

Well I see the information in PR description. However, issue keeps the analyze and PR focuses on the implementation. Please update the information in issue and it'd be better to label with number so that we know how much cases we'd like to cover.

Besides, I don't find a case matches the result of ALTER statement changes in the description.

yuqi1129 · 2021-07-10T09:30:27Z

Well I see the information in PR description. However, issue keeps the analyze and PR focuses on the implementation. Please update the information in issue and it'd be better to label with number so that we know how much cases we'd like to cover.

OK, i will make more description about this problem in the issue page

Besides, I don't find a case matches the result of ALTER statement changes in the description.

In Order to fix the problem , i change the stringToDecimal logic, which however affect the ALTER statements, for example
cast binary type to decimal will use stringToDecimal methods.

tisonkun · 2021-07-10T09:46:05Z

@yuqi1129 Thanks for your update. I'll schedule another review tomorrow.

tisonkun · 2021-07-11T09:15:23Z

@yuqi1129 I verify locally with MySQL 8.0.23 and behavior changes. I'd prefer discuss the expected behavior on the issue first.

tisonkun

I'm hesitated to approve this PR without enough background knowledge. Shall the other reviewers give significant suggestion.

tisonkun · 2021-07-12T12:26:31Z

ping @lzmhhh123 @wshwsh12

/cc @tangenta

wshwsh12

Looks like the behavior is even stranger now.... Now this sql will return two warning messages.

[tidb]> select cast('1.4a' as decimal); 
+-------------------------+
| cast('1.4a' as decimal) |
+-------------------------+
|                       1 |
+-------------------------+
1 row in set, 2 warnings (0.001 sec)

[tidb]> show warnings;
+---------+------+-------------------------------------------+
| Level   | Code | Message                                   |
+---------+------+-------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '1.4a' |
| Warning | 1292 | Truncated incorrect DECIMAL value: '1.4'  |
+---------+------+-------------------------------------------+
2 rows in set (0.000 sec)

In my opinion, we should not focus on fixing the inconsistency between these warning information and mysql at this stage, but other more serious problems (eg, returning wrong data).

wshwsh12 · 2021-07-19T08:29:31Z

expression/builtin_cast_vec.go

@@ -836,7 +836,7 @@ func (b *builtinCastRealAsDecimalSig) vecEvalDecimal(input *chunk.Chunk, result
 				if types.ErrOverflow.Equal(err) {
 					warnErr := types.ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", b.args[0])
 					err = b.ctx.GetSessionVars().StmtCtx.HandleOverflow(err, warnErr)
-				} else if types.ErrTruncated.Equal(err) {
+				} else if strings.Contains(err.Error(), "Truncated incorrect") {


Can we use types.ErrTruncatedWrongVal.Equal(err)?

Non-vec implement also need modify.

Are you sure we need modify CastRealAsDecimal, instead of CastStringAsDecimal?

Is there any related test? I didn't find the test cover the case.. Or maybe I ignore the test?

May also need to modify the implementation in tikv....

yuqi1129 · 2021-07-19T08:46:03Z

@wshwsh12

Looks like the behavior is even stranger now.... Now this sql will return two warning messages.

Just now, i am busy fix this, seems that in order to fix this, more code should be updated and change.

In my opinion, we should not focus on fixing the inconsistency between these warning information and mysql at this stage, but other more serious problems (eg, returning wrong data).

You are right, much attention has been put to this minor problem and the output is not of much importance. we can ignore warning info inconsistence currently and focus on more meaningful problem. i will close the PR and submit it in the future.

thanks for your kindly advice.

ti-chi-bot · 2021-09-21T20:32:51Z

@yuqi1129: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yuqi1129 added 3 commits June 30, 2021 20:07

executor: query memory table TIKV_REGION_PEERS and TIKV_REGION_STATUS…

4129fcb

… get error

Merge branch 'master' of github.com:pingcap/tidb

5c2c38c

expression: fix cast string like '.1a1' to decimal has no warnings in…

a8660e6

…fromation

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 6, 2021

Merge branch 'master' of github.com:pingcap/tidb into string_decimal

58dc37e

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 6, 2021

github-actions bot added the component/expression label Jul 6, 2021

ichn-hu mentioned this pull request Jul 7, 2021

Welcome to contribute #20804

Closed

yuqi1129 marked this pull request as draft July 7, 2021 06:09

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2021

expression: fix cast string like '.1a1' to decimal has no warnings in…

219c746

…fromation

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 7, 2021

Merge branch 'master' into string_decimal

141b0e8

yuqi1129 marked this pull request as ready for review July 7, 2021 12:45

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2021

yuqi1129 added 4 commits July 7, 2021 20:49

expression: fix cast string like '.1a1' to decimal has no warnings in…

e382660

…fromation

Merge remote-tracking branch 'me/string_decimal' into string_decimal

e9f1bcd

expression: fix cast string like '.1a1' to decimal has no warnings in…

e73d76f

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

7393274

…fromation

yuqi1129 force-pushed the string_decimal branch from 585da5f to 3a13ee6 Compare July 9, 2021 02:17

ti-chi-bot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 9, 2021

merge master

37c511f

yuqi1129 force-pushed the string_decimal branch from bbe88ff to 37c511f Compare July 9, 2021 02:33

yuqi1129 added 3 commits July 9, 2021 10:34

expression: fix cast string like '.1a1' to decimal has no warnings in…

66235fc

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

b9646b3

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

b20c17b

…fromation

github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 9, 2021

yuqi1129 added 5 commits July 9, 2021 19:20

expression: fix cast string like '.1a1' to decimal has no warnings in…

e4e4165

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

cc2277d

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

8f76d73

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

a65bd7b

…fromation

expression: fix cast string like '.1a1' to decimal has no warnings in…

05258c7

…fromation

ti-chi-bot requested review from lzmhhh123 and wshwsh12 July 10, 2021 08:52

tisonkun reviewed Jul 10, 2021

View reviewed changes

tisonkun self-requested a review July 10, 2021 09:45

tisonkun removed their request for review July 11, 2021 09:14

tisonkun reviewed Jul 12, 2021

View reviewed changes

ti-chi-bot requested a review from tangenta July 12, 2021 12:26

wshwsh12 reviewed Jul 19, 2021

View reviewed changes

yuqi1129 marked this pull request as draft July 19, 2021 08:47

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2021

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: fix cast string like '.1a1' to decimal has no warnings information #26005

expression: fix cast string like '.1a1' to decimal has no warnings information #26005

yuqi1129 commented Jul 6, 2021 •

edited

Loading

ti-chi-bot commented Jul 6, 2021

yuqi1129 commented Jul 10, 2021

tisonkun left a comment

tisonkun Jul 10, 2021

yuqi1129 Jul 10, 2021

tisonkun Jul 10, 2021

yuqi1129 Jul 10, 2021

tisonkun Jul 10, 2021

yuqi1129 Jul 10, 2021

wshwsh12 Jul 19, 2021 •

edited

Loading

tisonkun Jul 10, 2021

tisonkun Jul 10, 2021

tisonkun Jul 10, 2021

tisonkun Jul 10, 2021

tisonkun Jul 10, 2021

tisonkun commented Jul 10, 2021 •

edited

Loading

yuqi1129 commented Jul 10, 2021 •

edited

Loading

tisonkun commented Jul 10, 2021

tisonkun commented Jul 11, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading

tisonkun commented Jul 12, 2021

wshwsh12 left a comment

wshwsh12 Jul 19, 2021 •

edited

Loading

yuqi1129 commented Jul 19, 2021

ti-chi-bot commented Sep 21, 2021

expression: fix cast string like '.1a1' to decimal has no warnings information #26005

Are you sure you want to change the base?

expression: fix cast string like '.1a1' to decimal has no warnings information #26005

Conversation

yuqi1129 commented Jul 6, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Jul 6, 2021

yuqi1129 commented Jul 10, 2021

tisonkun left a 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

wshwsh12 Jul 19, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Jul 10, 2021 • edited Loading

yuqi1129 commented Jul 10, 2021 • edited Loading

tisonkun commented Jul 10, 2021

tisonkun commented Jul 11, 2021 • edited Loading

tisonkun left a comment • edited Loading

Choose a reason for hiding this comment

tisonkun commented Jul 12, 2021

wshwsh12 left a comment

Choose a reason for hiding this comment

wshwsh12 Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

yuqi1129 commented Jul 19, 2021

ti-chi-bot commented Sep 21, 2021

yuqi1129 commented Jul 6, 2021 •

edited

Loading

wshwsh12 Jul 19, 2021 •

edited

Loading

tisonkun commented Jul 10, 2021 •

edited

Loading

yuqi1129 commented Jul 10, 2021 •

edited

Loading

tisonkun commented Jul 11, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading

wshwsh12 Jul 19, 2021 •

edited

Loading