-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780
Changes from 3 commits
7762b8b
f87e0af
465dd1b
17f1e5f
1d15330
a803f9d
d6e6c25
ae342fc
869e1f6
b305a4f
bedd270
0e5de09
4045075
636170d
33bf09c
eb58c48
b199772
6005a18
e2aa622
d0395bf
ffc0df7
27202d6
03dc0e5
7f01b42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ | |
package expression | ||
|
||
import ( | ||
"math" | ||
|
||
"fmt" | ||
|
||
"github.com/juju/errors" | ||
"github.com/pingcap/tidb/context" | ||
"github.com/pingcap/tidb/parser/opcode" | ||
|
@@ -353,7 +357,15 @@ func (b *builtinUnaryOpSig) eval(row []types.Datum) (d types.Datum, err error) { | |
case types.KindInt64: | ||
d.SetInt64(-aDatum.GetInt64()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess opcode.Minus operation on |
||
case types.KindUint64: | ||
d.SetInt64(-int64(aDatum.GetUint64())) | ||
uval := aDatum.GetUint64() | ||
minInt64 := math.MinInt64 | ||
absMinInt64 := uint64(-minInt64) // 9223372036854775808 | ||
if uval > absMinInt64 { | ||
// -uval will overflow | ||
err = types.ErrOverflow.GenByArgs("BIGINT", fmt.Sprintf("-%v", uval)) | ||
} else { | ||
d.SetInt64(-int64(uval)) | ||
} | ||
case types.KindFloat64: | ||
d.SetFloat64(-aDatum.GetFloat64()) | ||
case types.KindFloat32: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright 2015 PingCAP, Inc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2015 -> 2017 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shenli done |
||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package expression | ||
|
||
import ( | ||
. "github.com/pingcap/check" | ||
"github.com/pingcap/tidb/ast" | ||
"github.com/pingcap/tidb/mysql" | ||
"github.com/pingcap/tidb/util/testleak" | ||
) | ||
|
||
func (s *testEvaluatorSuite) TestUnary(c *C) { | ||
defer testleak.AfterTest(c)() | ||
cases := []struct { | ||
args interface{} | ||
expected interface{} | ||
expectedType byte | ||
overflow bool | ||
getErr bool | ||
}{ | ||
{uint64(9223372036854775809), "-9223372036854775809", mysql.TypeNewDecimal, true, false}, | ||
{uint64(9223372036854775810), "-9223372036854775810", mysql.TypeNewDecimal, true, false}, | ||
{uint64(9223372036854775808), "-9223372036854775808", mysql.TypeLonglong, false, false}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the result of |
||
} | ||
|
||
for _, t := range cases { | ||
f, err := newFunctionForTest(s.ctx, ast.UnaryMinus, primitiveValsToConstants([]interface{}{t.args})...) | ||
|
||
c.Assert(err, IsNil) | ||
d, err := f.Eval(nil) | ||
if t.getErr { | ||
c.Assert(err, NotNil) | ||
} else { | ||
c.Assert(err, IsNil) | ||
if !t.overflow { | ||
c.Assert(d.GetString(), Equals, t.expected) | ||
} else { | ||
c.Assert(d.GetMysqlDecimal().String(), Equals, t.expected) | ||
c.Assert(f.GetType().Tp, Equals, t.expectedType) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"github.com/pingcap/tidb/model" | ||
"github.com/pingcap/tidb/mysql" | ||
"github.com/pingcap/tidb/sessionctx/variable" | ||
"github.com/pingcap/tidb/terror" | ||
"github.com/pingcap/tidb/util/codec" | ||
"github.com/pingcap/tidb/util/types" | ||
) | ||
|
@@ -157,12 +158,39 @@ func (sf *ScalarFunction) Decorrelate(schema *Schema) Expression { | |
return sf | ||
} | ||
|
||
func (sf *ScalarFunction) convertArgsToDecimal(sc *variable.StatementContext) error { | ||
ft := types.NewFieldType(mysql.TypeNewDecimal) | ||
for _, arg := range sf.GetArgs() { | ||
if constArg, ok := arg.(*Constant); ok { | ||
val, err := constArg.Value.ConvertTo(sc, ft) | ||
if err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. errors.Trace(err) |
||
} | ||
constArg.Value = val | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// Eval implements Expression interface. | ||
func (sf *ScalarFunction) Eval(row []types.Datum) (d types.Datum, err error) { | ||
sc := sf.GetCtx().GetSessionVars().StmtCtx | ||
if !TurnOnNewExprEval { | ||
return sf.Function.eval(row) | ||
d, err = sf.Function.eval(row) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any difference? |
||
// TODO: fix #3762, maybe better way | ||
if err != nil && terror.ErrorEqual(err, types.ErrOverflow) && sf.GetTypeClass() == types.ClassInt && | ||
sf.FuncName.L == ast.UnaryMinus && !sc.InUpdateOrDeleteStmt { | ||
err = sf.convertArgsToDecimal(sc) | ||
if err != nil { | ||
return d, errors.Trace(err) | ||
} | ||
d, err = sf.Function.eval(row) | ||
// change return type | ||
types.DefaultTypeForValue(d, sf.RetType) | ||
} | ||
return | ||
} | ||
sc := sf.GetCtx().GetSessionVars().StmtCtx | ||
var ( | ||
res interface{} | ||
isNull bool | ||
|
@@ -172,6 +200,7 @@ func (sf *ScalarFunction) Eval(row []types.Datum) (d types.Datum, err error) { | |
case types.ClassInt: | ||
var intRes int64 | ||
intRes, isNull, err = sf.EvalInt(row, sc) | ||
|
||
if mysql.HasUnsignedFlag(tp.Flag) { | ||
res = uint64(intRes) | ||
} else { | ||
|
@@ -191,6 +220,18 @@ func (sf *ScalarFunction) Eval(row []types.Datum) (d types.Datum, err error) { | |
res, isNull, err = sf.EvalString(row, sc) | ||
} | ||
} | ||
|
||
if err != nil && terror.ErrorEqual(err, types.ErrOverflow) && | ||
sf.FuncName.L == ast.UnaryMinus && !sc.InUpdateOrDeleteStmt { | ||
// TODO: fix #3762, overflow convert it to decimal | ||
err = sf.convertArgsToDecimal(sc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reference to this rule? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, mysql did not have any document refer to it, but there is some relevent doc: https://dev.mysql.com/doc/refman/5.7/en/out-of-range-and-overflow.html, |
||
if err != nil { | ||
return d, errors.Trace(err) | ||
} | ||
res, isNull, err = sf.EvalDecimal(row, sc) | ||
types.DefaultTypeForValue(res, sf.RetType) | ||
} | ||
|
||
if isNull || err != nil { | ||
d.SetValue(nil) | ||
return d, errors.Trace(err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the empty line.