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

fix compatibility bug with convert string to int return wrong result #7483

Merged
merged 8 commits into from
Aug 31, 2018
Merged

fix compatibility bug with convert string to int return wrong result #7483

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b9fbedc
fix compatibility bug with convert string to int return wrong result
CodeRushing Aug 24, 2018
dca12b6
Merge branch 'master' into my
ngaut Aug 24, 2018
4995e15
types: change AppendError to AppendWarning when convert to int overflows
CodeRushing Aug 27, 2018
41ef134
Merge branch 'master' into my
zz-jason Aug 27, 2018
da7a1a6
types:update comments in floatStrToIntStr when extraZeroCount exceed 20
CodeRushing Aug 28, 2018
42aaffb
Merge branch 'master' into my
zz-jason Aug 28, 2018
02d161d
Merge branch 'master' into my
Aug 31, 2018
1b5e5cc
Merge branch 'master' into my
Aug 31, 2018
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
40 changes: 40 additions & 0 deletions expression/builtin_cast_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,26 @@ func (s *testEvaluatorSuite) TestCast(c *C) {
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrTruncatedWrongVal, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// cast('125e342.83' as unsigned)
f = BuildCastFunction(ctx, &Constant{Value: types.NewDatum("125e342.83"), RetType: types.NewFieldType(mysql.TypeString)}, tp1)
res, err = f.Eval(chunk.Row{})
c.Assert(err, IsNil)
c.Assert(res.GetUint64() == 125, IsTrue)

warnings = sc.GetWarnings()
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrOverflow, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// cast('1e9223372036854775807' as unsigned)
f = BuildCastFunction(ctx, &Constant{Value: types.NewDatum("1e9223372036854775807"), RetType: types.NewFieldType(mysql.TypeString)}, tp1)
res, err = f.Eval(chunk.Row{})
c.Assert(err, IsNil)
c.Assert(res.GetUint64() == 1, IsTrue)

warnings = sc.GetWarnings()
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrOverflow, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// cast('18446744073709551616' as signed);
mask := ^mysql.UnsignedFlag
tp1.Flag &= uint(mask)
Expand All @@ -149,6 +169,26 @@ func (s *testEvaluatorSuite) TestCast(c *C) {
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrCastAsSignedOverflow, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// cast('125e342.83' as signed)
f = BuildCastFunction(ctx, &Constant{Value: types.NewDatum("125e342.83"), RetType: types.NewFieldType(mysql.TypeString)}, tp1)
res, err = f.Eval(chunk.Row{})
c.Assert(err, IsNil)
c.Assert(res.GetInt64() == 125, IsTrue)

warnings = sc.GetWarnings()
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrOverflow, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// cast('1e9223372036854775807' as signed)
f = BuildCastFunction(ctx, &Constant{Value: types.NewDatum("1e9223372036854775807"), RetType: types.NewFieldType(mysql.TypeString)}, tp1)
res, err = f.Eval(chunk.Row{})
c.Assert(err, IsNil)
c.Assert(res.GetInt64() == 1, IsTrue)

warnings = sc.GetWarnings()
lastWarn = warnings[len(warnings)-1]
c.Assert(terror.ErrorEqual(types.ErrOverflow, lastWarn.Err), IsTrue, Commentf("err %v", lastWarn.Err))

// create table t1(s1 time);
// insert into t1 values('11:11:11');
// select cast(s1 as decimal(7, 2)) from t1;
Expand Down
10 changes: 6 additions & 4 deletions types/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,13 @@ func getValidIntPrefix(sc *stmtctx.StatementContext, str string) (string, error)
if err != nil {
return floatPrefix, errors.Trace(err)
}
return floatStrToIntStr(floatPrefix)
return floatStrToIntStr(sc, floatPrefix, str)
}

// floatStrToIntStr converts a valid float string into valid integer string which can be parsed by
// strconv.ParseInt, we can't parse float first then convert it to string because precision will
// be lost.
func floatStrToIntStr(validFloat string) (string, error) {
func floatStrToIntStr(sc *stmtctx.StatementContext, validFloat string, oriStr string) (string, error) {
var dotIdx = -1
var eIdx = -1
for i := 0; i < len(validFloat); i++ {
Expand Down Expand Up @@ -275,7 +275,8 @@ func floatStrToIntStr(validFloat string) (string, error) {
}
if exp > 0 && int64(intCnt) > (math.MaxInt64-int64(exp)) {
// (exp + incCnt) overflows MaxInt64.
return validFloat, ErrOverflow.GenByArgs("BIGINT", validFloat)
sc.AppendError(ErrOverflow.GenByArgs("BIGINT", oriStr))
Copy link
Member

Choose a reason for hiding this comment

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

This means the previous error message is wrong?

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, the error message in this case is like below

mysql> show warnings;
+---------+------+-------------------------------------------------+
| Level | Code | Message |
+---------+------+-------------------------------------------------+
| Warning | 1292 | Truncated incorrect INTEGER value: '125e342.83' |
+---------+------+-------------------------------------------------+

Copy link
Contributor

Choose a reason for hiding this comment

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

So why AppendError? Shouldn't we AppendWarning?

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, it's should be AppendWarning. Already Fixed

return validFloat[:eIdx], nil
}
intCnt += exp
if intCnt <= 0 {
Expand All @@ -292,7 +293,8 @@ func floatStrToIntStr(validFloat string) (string, error) {
extraZeroCount := intCnt - len(digits)
if extraZeroCount > 20 {
// Return overflow to avoid allocating too much memory.
Copy link
Member

Choose a reason for hiding this comment

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

Should we update this comment?

Copy link
Member

Choose a reason for hiding this comment

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

@CodeRushing Please update this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zz-jason already updated

return validFloat, ErrOverflow.GenByArgs("BIGINT", validFloat)
sc.AppendError(ErrOverflow.GenByArgs("BIGINT", oriStr))
return validFloat[:eIdx], nil
}
validInt = string(digits) + strings.Repeat("0", extraZeroCount)
}
Expand Down
13 changes: 9 additions & 4 deletions types/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,15 @@ func (s *testTypeConvertSuite) TestGetValidFloat(c *C) {
_, err := strconv.ParseFloat(prefix, 64)
c.Assert(err, IsNil)
}
_, err := floatStrToIntStr("1e9223372036854775807")
c.Assert(terror.ErrorEqual(err, ErrOverflow), IsTrue, Commentf("err %v", err))
_, err = floatStrToIntStr("1e21")
c.Assert(terror.ErrorEqual(err, ErrOverflow), IsTrue, Commentf("err %v", err))
floatStr, err := floatStrToIntStr(sc, "1e9223372036854775807", "1e9223372036854775807")
c.Assert(err, IsNil)
c.Assert(floatStr, Equals, "1")
floatStr, err = floatStrToIntStr(sc, "125e342", "125e342.83")
c.Assert(err, IsNil)
c.Assert(floatStr, Equals, "125")
floatStr, err = floatStrToIntStr(sc, "1e21", "1e21")
c.Assert(err, IsNil)
c.Assert(floatStr, Equals, "1")
}

// TestConvertTime tests time related conversion.
Expand Down