expression/types: fix decimal minus/round/multiple result #7001

lysu · 2018-07-05T08:50:01Z

What have you changed? (mandatory)

this PR mainly fix three questions:

minus sign(-x) direct change decimal's negative property. (relate to negative small decimal return unexcept result #6997)
change scale truncate action in decimal.round base on the result of expression's decimal type infer
fix decimal.round expression's decimals type infer which help 2nd fix
fix multiple decimal panic (relate to panic when multiply very big decimal with a very small decimal #6985 as 3deb8f5b7c9f54bc1a9a4df979b93a1c13a9c213)
ignore data truncated error for multiple operation
make pingcap/tidb-test#545 pass

fixes #6997, #6985

What are the type of the changes (mandatory)?

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested (mandatory)?

unit tests
integration tests(pingcap/tidb-test#545)

This change is

shenli · 2018-07-05T08:53:49Z

server/util.go

@@ -254,7 +254,7 @@ func dumpBinaryRow(buffer []byte, columns []*ColumnInfo, row types.Row) ([]byte,
 		case mysql.TypeDouble:
 			buffer = dumpUint64(buffer, math.Float64bits(row.GetFloat64(i)))
 		case mysql.TypeNewDecimal:
-			buffer = dumpLengthEncodedString(buffer, hack.Slice(row.GetMyDecimal(i).String()))
+			buffer = dumpLengthEncodedString(buffer, hack.Slice(string(row.GetMyDecimal(i).ToString())))


Why change this?

mmm...In MySQL this sql will not round up to zero....

select -0.0000000000000000000000000000000000000000000000000017382578996420603

so I try to run test case ahead in here to see what happen if we
don't truncate it- -

coocood · 2018-07-05T12:13:29Z

types/mydecimal.go

+	if from.IsZero() {
+		return &to
+	}
+	if from.IsNegative() {


How about to.negative = !from.negative?

crazycs520 · 2018-07-05T13:09:45Z

server/util.go

@@ -254,7 +254,7 @@ func dumpBinaryRow(buffer []byte, columns []*ColumnInfo, row types.Row) ([]byte,
 		case mysql.TypeDouble:
 			buffer = dumpUint64(buffer, math.Float64bits(row.GetFloat64(i)))
 		case mysql.TypeNewDecimal:
-			buffer = dumpLengthEncodedString(buffer, hack.Slice(row.GetMyDecimal(i).String()))
+			buffer = dumpLengthEncodedString(buffer, hack.Slice(string(row.GetMyDecimal(i).ToString())))


buffer = dumpLengthEncodedString(buffer, row.GetMyDecimal(i).ToString())

good idea~finally we revert it and use continue call round up.- -

crazycs520 · 2018-07-05T13:10:02Z

server/util.go

@@ -312,7 +312,7 @@ func dumpTextRow(buffer []byte, columns []*ColumnInfo, row types.Row) ([]byte, e
 			tmp = appendFormatFloat(tmp[:0], row.GetFloat64(i), prec, 64)
 			buffer = dumpLengthEncodedString(buffer, tmp)
 		case mysql.TypeNewDecimal:
-			buffer = dumpLengthEncodedString(buffer, hack.Slice(row.GetMyDecimal(i).String()))
+			buffer = dumpLengthEncodedString(buffer, hack.Slice(string(row.GetMyDecimal(i).ToString())))


lysu · 2018-07-06T06:38:02Z

expression/builtin_math.go

@@ -1768,7 +1769,7 @@ func (b *builtinTruncateDecimalSig) evalDecimal(row types.Row) (*types.MyDecimal
 	}

 	result := new(types.MyDecimal)
-	if err := x.Round(result, int(d), types.ModeTruncate); err != nil {
+	if err := x.Round(result, mathutil.Min(int(d), b.getRetTp().Decimal), types.ModeTruncate); err != nil {


Truncate's param is given by user, so need check dec < tp.Decimal(which is less than MAX_DECIMAL_SCALE)

e.g. Truncate(1.124, 100)(we had test case).

lysu · 2018-07-06T06:47:42Z

types/mydecimal.go

@@ -713,9 +713,6 @@ func (d *MyDecimal) doMiniRightShift(shift, beg, end int) {
 // RETURN VALUE
 //  eDecOK/eDecTruncated
 func (d *MyDecimal) Round(to *MyDecimal, frac int, roundMode RoundMode) (err error) {
-	if frac > mysql.MaxDecimalScale {
-		frac = mysql.MaxDecimalScale
-	}


move this check out of mydecimal, and not > mysql.MaxDecimalScale but > e.tp.Decimals

lysu · 2018-07-07T13:06:12Z

expression/builtin_arithmetic.go

@@ -523,7 +524,7 @@ func (s *builtinArithmeticMultiplyDecimalSig) evalDecimal(row types.Row) (*types
 	}
 	c := &types.MyDecimal{}
 	err = types.DecimalMul(a, b, c)
-	if err != nil {
+	if err != nil && !terror.ErrorEqual(err, types.ErrTruncated) {


conventions: decimal_smth() <= 1 -- result is usable, but precision loss is possible

In MySQL multiple will ignore mul DataTruncated error without any error or warning.....

Do we have some test to cover this behavior?

yes~ 0151f10 add a new case, today.

select 2.00000000000000000000000000000001 * 1.000000000000000000000000000000000000000000002 will failed in master, but pass in this PR and mysql with 2.000000000000000000000000000000

lysu · 2018-07-07T13:21:06Z

/run-all-tests tidb-test=pr/545

lysu · 2018-07-07T14:09:00Z

/run-unit-test tidb-test=pr/545

lysu · 2018-07-08T06:50:02Z

PTAL @coocood @AndreMouche if free~ thx

coocood · 2018-07-09T11:19:10Z

types/mydecimal_test.go

@@ -626,7 +645,7 @@ func (s *testMyDecimalSuite) TestMul(c *C) {
 		{"123456", "987654321", "121931851853376", nil},
 		{"123456", "9876543210", "1219318518533760", nil},
 		{"123", "0.01", "1.23", nil},
-		{"123", "0", "0", nil},


Why remove the old test case?

It's my mistake.- -

In detail fix two question: - minus sign direct change decimal's negative property - write decimal's round no longer truncate scale param any more fixes #6997

coocood · 2018-07-10T08:11:47Z

LGTM

lysu · 2018-07-10T12:41:45Z

PTAL @AndreMouche @XuHuaiyu if free, thx~

zz-jason · 2018-07-11T08:41:44Z

expression/builtin_math.go

@@ -292,6 +295,25 @@ func (c *roundFunctionClass) getFunction(ctx sessionctx.Context, args []Expressi
 	return sig, nil
 }

+// fixDecimal4RoundAndTruncate fixes tp.decimals of round/truncate func.
+func fixDecimal4RoundAndTruncate(ctx sessionctx.Context, args []Expression, retType types.EvalType) int {


s/fix.../calc.../

zz-jason · 2018-07-11T08:43:42Z

expression/builtin_math.go

+		return args[0].GetType().Decimal
+	}
+	argDec, isNull, err := secondConst.EvalInt(ctx, nil)
+	if isNull || err != nil || argDec < 0 {


we should check err firstly

zz-jason · 2018-07-11T15:35:44Z

/run-all-tests

zz-jason

LGTM 👍

lysu added the type/compatibility label Jul 5, 2018

lysu requested review from winkyao, coocood and AndreMouche July 5, 2018 08:50

shenli reviewed Jul 5, 2018

View reviewed changes

lysu added the status/DNM label Jul 5, 2018

coocood reviewed Jul 5, 2018

View reviewed changes

crazycs520 reviewed Jul 5, 2018

View reviewed changes

lysu commented Jul 6, 2018

View reviewed changes

lysu commented Jul 7, 2018

View reviewed changes

lysu removed the status/DNM label Jul 7, 2018

lysu changed the title ~~expression/types: fix negative small decimal bug.~~ expression/types: fix decimal minus/round/multiple result Jul 7, 2018

lysu added the status/all tests passed label Jul 7, 2018

lysu assigned coocood Jul 9, 2018

coocood reviewed Jul 9, 2018

View reviewed changes

lysu added 6 commits July 9, 2018 21:31

expression/types: fix negative small decimal bug.

6668431

In detail fix two question: - minus sign direct change decimal's negative property - write decimal's round no longer truncate scale param any more fixes #6997

fix decimal derive in round/truncate.

2ef808c

fix testcase..maybe need more fix.

7bba6ba

make testcase back to right.

3de2dd1

fix issue #6985 that decimal mul cause panic

039feff

fix comment, should not remove old case.

126d875

coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 10, 2018

zz-jason self-assigned this Jul 11, 2018

add testcase for decimal mul datatrucate

0151f10

zz-jason reviewed Jul 11, 2018

View reviewed changes

lysu and others added 2 commits July 11, 2018 23:29

address comment~

f36ed84

Merge branch 'master' into dev-decimal-mul-fix

7d1dbcd

zz-jason approved these changes Jul 11, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. component/expression and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 11, 2018

zz-jason merged commit 5f7fc80 into pingcap:master Jul 11, 2018

lysu deleted the dev-decimal-mul-fix branch July 12, 2018 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression/types: fix decimal minus/round/multiple result #7001

expression/types: fix decimal minus/round/multiple result #7001

lysu commented Jul 5, 2018 •

edited

Loading

shenli Jul 5, 2018

lysu Jul 5, 2018 •

edited

Loading

coocood Jul 5, 2018

lysu Jul 6, 2018

crazycs520 Jul 5, 2018

lysu Jul 6, 2018

crazycs520 Jul 5, 2018

lysu Jul 6, 2018

lysu Jul 6, 2018 •

edited

Loading

lysu Jul 7, 2018

zz-jason Jul 10, 2018

lysu Jul 11, 2018 •

edited

Loading

lysu commented Jul 7, 2018

lysu commented Jul 7, 2018

lysu commented Jul 8, 2018

coocood Jul 9, 2018

lysu Jul 9, 2018

coocood commented Jul 10, 2018

lysu commented Jul 10, 2018

zz-jason Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason commented Jul 11, 2018

zz-jason left a comment •

edited

Loading

expression/types: fix decimal minus/round/multiple result #7001

expression/types: fix decimal minus/round/multiple result #7001

Conversation

lysu commented Jul 5, 2018 • edited Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Choose a reason for hiding this comment

lysu Jul 5, 2018 • 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

lysu Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

lysu commented Jul 7, 2018

lysu commented Jul 7, 2018

lysu commented Jul 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Jul 10, 2018

lysu commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Jul 11, 2018

zz-jason left a comment • edited Loading

Choose a reason for hiding this comment

lysu commented Jul 5, 2018 •

edited

Loading

lysu Jul 5, 2018 •

edited

Loading

lysu Jul 6, 2018 •

edited

Loading

lysu Jul 11, 2018 •

edited

Loading

zz-jason left a comment •

edited

Loading