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.

Sign up for GitHub

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

Jump to bottom

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

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4129fcb
executor: query memory table TIKV_REGION_PEERS and TIKV_REGION_STATUS…
yuqi1129 Jun 30, 2021
5c2c38c
Merge branch 'master' of github.com:pingcap/tidb
yuqi1129 Jul 1, 2021
a8660e6
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 6, 2021
58dc37e
Merge branch 'master' of github.com:pingcap/tidb into string_decimal
yuqi1129 Jul 6, 2021
219c746
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 7, 2021
141b0e8
Merge branch 'master' into string_decimal
yuqi1129 Jul 7, 2021
e382660
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 7, 2021
e9f1bcd
Merge remote-tracking branch 'me/string_decimal' into string_decimal
yuqi1129 Jul 7, 2021
e73d76f
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 8, 2021
7393274
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
f0ed397
Merge branch 'master' of github.com:pingcap/tidb
yuqi1129 Jul 9, 2021
37c511f
merge master
yuqi1129 Jul 9, 2021
66235fc
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
b9646b3
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
b20c17b
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
e4e4165
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
cc2277d
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
8f76d73
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
a65bd7b
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
05258c7
expression: fix cast string like '.1a1' to decimal has no warnings in…
yuqi1129 Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ddl/column_type_change_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, In MySQL, this case will directly return error

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"))
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, it seems result changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see the reason above

// double
reset(tk)
tk.MustExec("insert into t values ('123.45', '123.45', '123.45', '123.45', '123.45', '123.45', '123', '123')")
Expand Down
2 changes: 1 addition & 1 deletion expression/builtin_cast_vec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@wshwsh12 wshwsh12 Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Can we use types.ErrTruncatedWrongVal.Equal(err)?
  2. Non-vec implement also need modify.
  3. Are you sure we need modify CastRealAsDecimal, instead of CastStringAsDecimal?
  4. Is there any related test? I didn't find the test cover the case.. Or maybe I ignore the test?
  5. May also need to modify the implementation in tikv....

// This behavior is consistent with MySQL.
err = nil
}
Expand Down
29 changes: 29 additions & 0 deletions expression/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2723,6 +2723,35 @@ func (s *testIntegrationSuite2) TestBuiltin(c *C) {
result.Check(testkit.Rows("9223372036854775808", "9223372036854775808"))
tk.MustExec(`drop table tb5`)

// test builtinCastStringAsDecimalSig
tk.MustExec(`drop table if exists tb5`)
tk.MustExec(`create table tb5 (a varchar(20));`)
tk.MustExec(`insert into tb5 values ('123'), ('.0a1');`)
result = tk.MustQuery(`select cast(a as decimal(10, 2)) from tb5;`)
result.Check(testkit.Rows("123.00", "0.00"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '.0a1'"))

result = tk.MustQuery("select cast('123E5a' as decimal(10,2));")
result.Check(testkit.Rows("12300000.00"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '123E5a'"))

result = tk.MustQuery(`select cast('123aE5' as decimal(10, 2));`)
result.Check(testkit.Rows("123.00"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '123aE5'"))

result = tk.MustQuery(`select cast('1.4a' as decimal(10, 2));`)
result.Check(testkit.Rows("1.40"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '1.4a'"))

result = tk.MustQuery(`select cast('1e - 1' as decimal);`)
result.Check(testkit.Rows("1"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '1e - 1'"))

result = tk.MustQuery(`select cast('1 1' as decimal)`)
result.Check(testkit.Rows("1"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect DECIMAL value: '1 1'"))
tk.MustExec(`drop table tb5`)

// test builtinCastIntAsRealSig
tk.MustExec(`drop table if exists tb5`)
tk.MustExec(`create table tb5 (a bigint(64) unsigned, b double(64, 10));`)
Expand Down
5 changes: 3 additions & 2 deletions types/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto behavior changes

ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1,999.00")), IsTrue, Commentf("err %v", err))
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto behavior changes

c.Assert(v.(*MyDecimal).String(), Equals, "1.0000")
v, err = Convert("199.00 ", ft)
c.Assert(err, IsNil)
Expand Down
36 changes: 32 additions & 4 deletions types/mydecimal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,15 +414,24 @@ func (d *MyDecimal) ToString() (str []byte) {
func (d *MyDecimal) FromString(str []byte) error {
// strErr is used to check str is bad number or not
var strErr error
var strBak = str
for i := 0; i < len(str); i++ {
if !isSpace(str[i]) {
str = str[i:]
break
}
}
// trim space in the end
for j := len(str) - 1; j >= 0; j-- {
if !isSpace(str[j]) {
str = str[:j+1]
break
}
}

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

Choose a reason for hiding this comment

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

ditto behavior changes

}
switch str[0] {
case '-':
Expand All @@ -443,9 +452,13 @@ func (d *MyDecimal) FromString(str []byte) error {
for endIdx < len(str) && isDigit(str[endIdx]) {
endIdx++
}

if endIdx < len(str) && str[endIdx] != 'E' && str[endIdx] != 'e' && str[endIdx] != ' ' {
strErr = ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(strBak))
}
digitsFrac = endIdx - strIdx - 1
} else if strIdx < len(str) && (str[strIdx] != 'e' && str[strIdx] != 'E' && str[strIdx] != ' ') {
strErr = ErrBadNumber
strErr = ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(strBak))
} else {
digitsFrac = 0
endIdx = strIdx
Expand Down Expand Up @@ -481,6 +494,11 @@ func (d *MyDecimal) FromString(str []byte) error {
innerIdx = 0
}
}

if digitsInt != 0 {
strErr = ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(strBak))
}

if innerIdx != 0 {
wordIdx--
d.wordBuf[wordIdx] = word
Expand Down Expand Up @@ -508,8 +526,8 @@ func (d *MyDecimal) FromString(str []byte) error {
if endIdx+1 <= len(str) && (str[endIdx] == 'e' || str[endIdx] == 'E') {
exponent, err1 := strToInt(string(str[endIdx+1:]))
if err1 != nil {
err = errors.Cause(err1)
if err != ErrTruncated {
err = err1
if errors.Cause(err1) != ErrTruncated {
*d = zeroMyDecimal
}
}
Expand All @@ -535,6 +553,15 @@ func (d *MyDecimal) FromString(str []byte) error {
}
}
}

if err == ErrTruncated {
strErr = ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(str))
}

if strErr == nil && (endIdx+1 < len(str) && str[endIdx] == ' ') {
strErr = ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", string(str))
}

allZero := true
for i := 0; i < wordBufLen; i++ {
if d.wordBuf[i] != 0 {
Expand All @@ -549,6 +576,7 @@ func (d *MyDecimal) FromString(str []byte) error {
if strErr != nil {
return strErr
}

Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid unnecessary diff.

return err
}

Expand Down
30 changes: 20 additions & 10 deletions types/mydecimal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"testing"

. "github.com/pingcap/check"
"github.com/pingcap/parser/terror"
)

var _ = Suite(&testMyDecimalSuite{})
Expand Down Expand Up @@ -533,8 +534,8 @@ func (s *testMyDecimalSerialSuite) TestFromString(c *C) {
tests := []tcase{
{"12345", "12345", nil},
{"12345.", "12345", nil},
{"123.45.", "123.45", nil},
{"-123.45.", "-123.45", nil},
{"123.45.", "123.45", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "123.45.")},
{"-123.45.", "-123.45", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "-123.45.")},
{".00012345000098765", "0.00012345000098765", nil},
{".12345000098765", "0.12345000098765", nil},
{"-.000000012345000098765", "-0.000000012345000098765", nil},
Expand All @@ -544,31 +545,40 @@ func (s *testMyDecimalSerialSuite) TestFromString(c *C) {
{"1e1073741823", "999999999999999999999999999999999999999999999999999999999999999999999999999999999", ErrOverflow},
{"-1e1073741823", "-999999999999999999999999999999999999999999999999999999999999999999999999999999999", ErrOverflow},
{"1e18446744073709551620", "0", ErrBadNumber},
{"1e", "1", ErrTruncated},
{"1e", "1", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1e")},
{"1e001", "10", nil},
{"1e00", "1", nil},
{"1eabc", "1", ErrTruncated},
{"1e 1dddd ", "10", ErrTruncated},
{"1e - 1", "1", ErrTruncated},
{"1eabc", "1", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1eabc")},
{"1e 1dddd ", "10", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1e 1dddd ")},
{"1e - 1", "1", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1e - 1")},
{"1e -1", "0.1", nil},
{"0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "0.000000000000000000000000000000000000000000000000000000000000000000000000", ErrTruncated},
{"0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"0.000000000000000000000000000000000000000000000000000000000000000000000000",
ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL",
"0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000")},
{"0.1a2", "0.1", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "0.1a2")},
{"123aE5", "123", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "123aE5")},
{"123E5a", "12300000", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "123E5a")},
{"1 1", "1", ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "1 1")},
}
for _, ca := range tests {
var dec MyDecimal
err := dec.FromString([]byte(ca.input))
c.Check(err, Equals, ca.err, Commentf("input: %s", ca.input))

c.Assert(terror.ErrorEqual(err, ca.err), IsTrue, Commentf("input: %s", ca.input))
result := dec.ToString()
c.Check(string(result), Equals, ca.output, Commentf("dec:%s", dec.String()))
}
wordBufLen = 1
tests = []tcase{
{"123450000098765", "98765", ErrOverflow},
{"123450.000098765", "123450", ErrTruncated},
{"123450.000098765", "123450",
ErrTruncatedWrongVal.GenWithStackByArgs("DECIMAL", "123450.000098765")},
}
for _, ca := range tests {
var dec MyDecimal
err := dec.FromString([]byte(ca.input))
c.Check(err, Equals, ca.err)
c.Assert(terror.ErrorEqual(err, ca.err), IsTrue)
result := dec.ToString()
c.Check(string(result), Equals, ca.output, Commentf("dec:%s", dec.String()))
}
Expand Down
3 changes: 2 additions & 1 deletion types/parser_driver/value_expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto brittle string matching.

strings.Contains(err.Error(), "Truncated incorrect")) {
err = nil
}
return dec, err
Expand Down