expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780

winkyao · 2017-07-17T09:56:18Z

…scalar function

shenli · 2017-07-17T09:58:00Z

expression/builtin_op.go

@@ -14,6 +14,10 @@
 package expression

 import (
+	"math"
+


Remove the empty line.

shenli · 2017-07-17T09:58:30Z

expression/builtin_op_test.go

@@ -0,0 +1,54 @@
+// Copyright 2015 PingCAP, Inc.


2015 -> 2017

@shenli done

tiancaiamao · 2017-07-18T02:44:18Z

expression/builtin_op.go

@@ -353,7 +356,15 @@ func (b *builtinUnaryOpSig) eval(row []types.Datum) (d types.Datum, err error) {
 		case types.KindInt64:
 			d.SetInt64(-aDatum.GetInt64())


I guess opcode.Minus operation on math.MinInt64 would get wrong result as range of negative is greater then range of positive by 1.

tiancaiamao · 2017-07-18T02:55:50Z

expression/builtin_op_test.go

+	}{
+		{uint64(9223372036854775809), "-9223372036854775809", mysql.TypeNewDecimal, true, false},
+		{uint64(9223372036854775810), "-9223372036854775810", mysql.TypeNewDecimal, true, false},
+		{uint64(9223372036854775808), "-9223372036854775808", mysql.TypeLonglong, false, false},


what's the result of int64(-9223372036854775808) ?

…s 2, should return datum, not row

tiancaiamao · 2017-07-21T10:31:44Z

expression/builtin_op.go

+	types.SetBinChsClnFlag(bf.tp)
+
+	overflow := false
+	if arg, ok := argExpr.(*Constant); ok {


Why check overflow in type infer? If expr is not Constant, this is useless.

@XuHuaiyu, because overflow will be handled only if the statement is Select, and if the args type is not constant, MySQL will just throw a overflow error out, see below:

mysql> select * from t; Field 1: `a` Catalog: `def` Database: `test` Table: `t` Org_table: `t` Type: LONGLONG Collation: binary (63) Length: 20 Max_length: 19 Decimals: 0 Flags: UNSIGNED NUM +---------------------+ | a | +---------------------+ | 9223372036854775809 | | 9223372036854775808 | +---------------------+ 2 rows in set (0.00 sec) mysql> select -a from t; ERROR 1690 (22003): BIGINT value is out of range in ‘-(`test`.`t`.`a`)’

tiancaiamao · 2017-07-21T10:34:36Z

expression/builtin_op.go

+	baseIntBuiltinFunc
+}
+
+// func (b *builtinUnaryMinusIntSig) eval(row []types.Datum) (d types.Datum, err error) {


This can be removed?

yes, this is not the final commit, cause some test case failed I can't fix, so I push the code to github, I will remove these code after problem fixed

tiancaiamao · 2017-07-21T10:36:28Z

expression/builtin_op.go

+	}
+
+	if mysql.HasUnsignedFlag(b.args[0].GetType().Flag) {
+		uval := uint64(val)


move the overflow check logic to EvalInt ?

@tiancaiamao the overflow logic is only for unary minus

tiancaiamao · 2017-07-21T10:45:15Z

expression/expression.go

@@ -170,7 +170,8 @@ func evalExprToDecimal(expr Expression, row []types.Datum, sc *variable.Statemen
 	if val.IsNull() || err != nil {
 		return res, val.IsNull(), errors.Trace(err)
 	}
-	if expr.GetTypeClass() == types.ClassDecimal {
+	switch expr.GetTypeClass() {
+	case types.ClassDecimal, types.ClassInt:


ClassInt evaluate to Decimal automatically?

yes, it will do it correctlly

tiancaiamao · 2017-07-21T10:46:22Z

expression/scalar_function.go

+		if constArg, ok := arg.(*Constant); ok {
+			val, err := constArg.Value.ConvertTo(sc, ft)
+			if err != nil {
+				return err


errors.Trace(err)

tiancaiamao · 2017-07-21T10:56:25Z

expression/scalar_function.go

+	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)


Is there any reference to this rule?
How about Add function and others?

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,

tiancaiamao · 2017-07-21T10:57:27Z

plan/expression_rewriter.go

@@ -93,7 +93,8 @@ func (b *planBuilder) rewriteWithPreprocess(expr ast.ExprNode, p LogicalPlan, ag
 	if getRowLen(er.ctxStack[0]) != 1 {
 		return nil, nil, ErrOperandColumns.GenByArgs(1)
 	}
-	result := expression.FoldConstant(er.ctxStack[0])
+	result := expression.PlainFoldConstant(er.ctxStack[0])
+	// result := expression.FoldConstant(er.ctxStack[0])


Remove it, keep the code clean.

yes, will clean it in final commit

tiancaiamao · 2017-07-21T10:59:52Z

plan/expression_rewriter.go

-	ret = &expression.Constant{Value: types.NewDatum(c.Value.GetRow()[1:]), RetType: c.GetType()}
+	if getRowLen(c) == 2 {
+		ret = &expression.Constant{Value: c.Value.GetRow()[1], RetType: c.GetType()}
+	} else {


why change here?

because if getRowLen(c) == 2, if return c.Value.GetRow()[1:], it will treat it as row, and build a row > row function, but it should be datum > datum in this case, see the

if f, ok := e.(*expression.ScalarFunction); ok { args := f.GetArgs() if len(args) == 2 { // in here, do the same thing return args[1].Clone(), nil } ret, err = expression.NewFunction(ctx, f.FuncName.L, f.GetType(), args[1:]...) return ret, errors.Trace(err) }

and if not change this, the test case select row(1, 1) > row(1, 0) will return 0, which should return result 1

hanfei1991 · 2017-07-23T06:34:47Z

expression/builtin_op.go

+	types.SetBinChsClnFlag(bf.tp)
+
+	overflow := false
+	if arg, ok := argExpr.(*Constant); ok {


It's weird here. Can we change parser to parse -9223372036854775809 as a constant decimal directly ?

no, image this case "--9223372036854775809". we should not do too much things in parser

…/fix_issue_3762

shenli · 2017-07-25T01:00:25Z

Is this PR still WIP?

winkyao · 2017-07-25T01:20:30Z

yes. still wip. @shenli

…o handle overflow gracely

winkyao · 2017-07-25T01:57:48Z

@shenli @tiancaiamao @XuHuaiyu @zz-jason @zimulala PTAL

XuHuaiyu · 2017-07-25T06:12:44Z

executor/aggregate_test.go

@@ -318,6 +318,7 @@ func (s *testSuite) TestAggPrune(c *C) {
 		testleak.AfterTest(c)()
 	}()
 	tk := testkit.NewTestKit(c, s.store)
+


useless line ？

XuHuaiyu · 2017-07-25T06:14:44Z

expression/builtin_op.go

+
+func (b *unaryMinusFunctionClass) typeInfer(argExpr Expression) (evalTp, bool) {
+	tp := tpInt
+	switch argExpr.GetType().Tp {


you can use argExpr.getClassType() here ?

case types.ClassString, types.ClassReal: tp = tpReal case types.ClassDecimal: tp = tpDecimal

XuHuaiyu · 2017-07-25T06:17:35Z

expression/builtin_op.go

+	case types.ClassString:
+		tp := argExpr.GetType().Tp
+		if types.IsTypeTime(tp) {
+			bf, err := newBaseBuiltinFuncWithTp(args, ctx, retTp, tpDecimal)


line 465 and ling 471 can be merged?

XuHuaiyu · 2017-07-25T06:20:43Z

expression/builtin_op.go

+		}
+	}
+
+	return sig.setSelf(sig), errors.Trace(b.verifyArgs(args))


we may put b.verifyArgs at the beginning of this func to check the count of args.

XuHuaiyu · 2017-07-25T06:23:28Z

expression/builtin_op.go

+	return sig.setSelf(sig), errors.Trace(b.verifyArgs(args))
+}
+
+func unaryMinusDecimal(dec *types.MyDecimal) (to *types.MyDecimal, err error) {


this func may be unnecessary?

XuHuaiyu · 2017-07-25T06:26:35Z

expression/constant_fold.go

+
+// PlainFoldConstant does constant folding optimization
+// on an expression without recursively folding.
+func PlainFoldConstant(expr Expression) Expression {


winkyao · 2017-07-25T07:45:22Z

@XuHuaiyu

XuHuaiyu · 2017-07-25T07:59:21Z

expression/builtin_op.go

@@ -338,9 +344,19 @@ func (b *builtinUnaryOpSig) eval(row []types.Datum) (d types.Datum, err error) {
 	case opcode.Minus:


the code will never reach here?

XuHuaiyu · 2017-07-25T08:02:54Z

expression/builtin_op.go

+	overflow := false
+	// TODO: handle float overflow
+	if arg, ok := argExpr.(*Constant); ok {
+		switch arg.Value.Kind() {


we may check whether mysql.HasUnsignedFlag(arg.GetType().Flag) here
rather than check arg.Value.Kind()

XuHuaiyu · 2017-07-25T08:03:47Z

expression/builtin_op.go

+		return nil, errors.Trace(err)
+	}
+
+	argExpr := args[0]


argExpr only used in line 438, we can just use args[0] instead of argExpr.

see line 441 and 469

XuHuaiyu · 2017-07-25T09:02:00Z

expression/builtin_op.go

+	// TODO: handle float overflow
+	if arg, ok := argExpr.(*Constant); ok {
+		switch arg.Value.Kind() {
+		case types.KindUint64:


comment for the two overflow check.

XuHuaiyu · 2017-07-25T09:05:47Z

expression/builtin_op.go

+
+	to := new(types.MyDecimal)
+	err = types.DecimalSub(new(types.MyDecimal), dec, to)
+	return to, false, err


s/err/errors.Trace(err)

XuHuaiyu · 2017-07-25T09:07:57Z

expression/scalar_function.go

 	if !TurnOnNewExprEval {
-		return sf.Function.eval(row)
+		d, err = sf.Function.eval(row)


any difference?

XuHuaiyu · 2017-07-25T09:09:38Z

expression/builtin_op_test.go

+		} else {
+			c.Assert(err, NotNil)
+		}
+	}


add test case for isDeterminstic.

…/fix_issue_3762

winkyao · 2017-07-25T16:33:50Z

@XuHuaiyu PTAL

XuHuaiyu

Rest LGTM

XuHuaiyu · 2017-07-26T02:11:42Z

expression/builtin_op.go

+}
+
+func (b *unaryMinusFunctionClass) handleIntOverflow(arg *Constant) (overflow bool) {
+	overflow = false


this is useless.

XuHuaiyu · 2017-07-26T02:12:58Z

expression/builtin_op.go

+		// -math.MinInt64 is 9223372036854775808, so if uval is more than 9223372036854775808, like
+		// 9223372036854775809, -9223372036854775809 is less than math.MinInt64, overflow occurs.
+		if uval > uint64(-math.MinInt64) {
+			overflow = true


just use return true here is enough,
since the return value has been named as overflow which is explicit.

the same as line 368

XuHuaiyu · 2017-07-26T02:14:19Z

expression/builtin_op.go

+	return
+}
+
+func (b *unaryMinusFunctionClass) typeInfer(argExpr Expression, ctx context.Context) (evalTp, bool) {


add a comment for what this function does.

XuHuaiyu · 2017-07-26T02:19:01Z

PTAL @zz-jason

into winkyao/fix_issue_3762

…/fix_issue_3762

into winkyao/fix_issue_3762

XuHuaiyu · 2017-07-26T02:55:58Z

expression/builtin_op.go

 }

+// typeInfer infer unary Minus Function return type. when arg is int constant and overflow,


s/ infer/ infers
s/ unary Minus/ unaryMinus
s/ Function/ function
s/ when arg/ When the arg
s/ int/ an int

XuHuaiyu · 2017-07-26T03:01:00Z

LGTM

hanfei1991

LGTM

winkyao added 3 commits July 17, 2017 11:13

expression: handle int builtinUnaryOpSig overflow

7762b8b

expression: fix #3762, signed integer overflow handle in minus unary …

f87e0af

…scalar function

expression: fix #3762, add builtin_op_test.go, some builtin test cases

465dd1b

shenli reviewed Jul 17, 2017

View reviewed changes

winkyao added 2 commits July 17, 2017 18:03

*: tiny cleanup

17f1e5f

expression: set the correct unary minus function ScalarFunction.RetType

1d15330

tiancaiamao reviewed Jul 18, 2017

View reviewed changes

XuHuaiyu added the WIP label Jul 18, 2017

winkyao added 2 commits July 19, 2017 13:33

expression: rewrite unary minus builtin function to handle overflow.

a803f9d

plan: fix popRowArg bug: when get expression.Constatn and getRowLen i…

d6e6c25

…s 2, should return datum, not row

tiancaiamao reviewed Jul 21, 2017

View reviewed changes

hanfei1991 reviewed Jul 23, 2017

View reviewed changes

*:git stash

ae342fc

hanfei1991 mentioned this pull request Jul 24, 2017

plan, expression: move constant fold to NewFunction #3868

Merged

Merge branch 'master' of https://github.com/pingcap/tidb into winkyao…

869e1f6

…/fix_issue_3762

expression: base #3868 pr, and rewrite builtin function unary minus t…

b305a4f

…o handle overflow gracely

winkyao added rc3.1 type/compatibility and removed WIP labels Jul 25, 2017

XuHuaiyu added the WIP label Jul 25, 2017

expression: refactor builtin unary minus function

bedd270

winkyao removed the WIP label Jul 25, 2017

XuHuaiyu reviewed Jul 25, 2017

View reviewed changes

expression: tiny refactor

0e5de09

expression: tiny cleanup

4045075

XuHuaiyu reviewed Jul 25, 2017

View reviewed changes

winkyao added 3 commits July 25, 2017 18:46

expression: cleanup

636170d

expresion: cleanup

33bf09c

expression: tiny cleanup

eb58c48

hanfei1991 added the conflicting label Jul 25, 2017

Merge branch 'master' of https://github.com/pingcap/tidb into winkyao…

b199772

…/fix_issue_3762

winkyao removed the conflicting label Jul 25, 2017

Merge branch 'master' into winkyao/fix_issue_3762

6005a18

XuHuaiyu reviewed Jul 26, 2017

View reviewed changes

winkyao and others added 3 commits July 26, 2017 10:35

expression: add some comment

e2aa622

Merge branch 'winkyao/fix_issue_3762' of https://github.com/pingcap/tidb

d0395bf

into winkyao/fix_issue_3762

Merge branch 'master' into winkyao/fix_issue_3762

ffc0df7

winkyao closed this Jul 26, 2017

winkyao reopened this Jul 26, 2017

winkyao added 2 commits July 26, 2017 10:38

Merge branch 'master' of https://github.com/pingcap/tidb into winkyao…

27202d6

…/fix_issue_3762

Merge branch 'winkyao/fix_issue_3762' of https://github.com/pingcap/tidb

03dc0e5

into winkyao/fix_issue_3762

XuHuaiyu reviewed Jul 26, 2017

View reviewed changes

XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 26, 2017

expression: adjust commet

7f01b42

hanfei1991 approved these changes Jul 26, 2017

View reviewed changes

hanfei1991 merged commit e008c6c into master Jul 26, 2017

hanfei1991 deleted the winkyao/fix_issue_3762 branch July 26, 2017 03:44

YangKeao mentioned this pull request Dec 4, 2023

Truncate error shouldn't be allowed when handling default values #49141

Open

		@@ -353,7 +356,15 @@ func (b *builtinUnaryOpSig) eval(row []types.Datum) (d types.Datum, err error) {
		case types.KindInt64:
		d.SetInt64(-aDatum.GetInt64())

		@@ -338,9 +344,19 @@ func (b *builtinUnaryOpSig) eval(row []types.Datum) (d types.Datum, err error) {
		case opcode.Minus:

		}

		// typeInfer infer unary Minus Function return type. when arg is int constant and overflow,

expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780

expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780

Conversation

winkyao commented Jul 17, 2017

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

Choose a reason for hiding this comment

winkyao Jul 21, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Jul 25, 2017

winkyao commented Jul 25, 2017

winkyao commented Jul 25, 2017

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

winkyao commented Jul 25, 2017

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

Choose a reason for hiding this comment

winkyao commented Jul 25, 2017

XuHuaiyu 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

XuHuaiyu commented Jul 26, 2017

Choose a reason for hiding this comment

XuHuaiyu commented Jul 26, 2017

hanfei1991 left a comment

Choose a reason for hiding this comment

winkyao Jul 21, 2017 •

edited

Loading