executor: avoid sum from avg overflow #30010

fzhedu · 2021-11-22T13:20:41Z

What problem does this PR solve?

Issue Number: close #29952

Problem Summary:

the sum from avg may overflow, we should infer the type as the normal sum.
the previous PR expression, planner: fix decimal results for aggregate functions #20017 is a hack to fix the issue , we should let the final agg output the target return type which is inferred in TypeInfe() for Agg.
typeInfer4xxx should valid the precision and scale of decimal type.

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-11-22T13:20:43Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lzmhhh123
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

windtalker · 2021-11-23T07:10:08Z

planner/core/task.go

@@ -1520,6 +1520,16 @@ type AggInfo struct {
 	Schema       *expression.Schema
 }

+// ExpandRetTypeForAvg expands the Flen of decimal type to avoid overflow


I think it is a little hack, why not just infer the schema for sum function? This will make sure that the rewriten sum share the same schema with original sum.

that should make the scope for typeInfer() to TypeInfer()

windtalker · 2021-11-23T08:23:44Z

planner/core/task.go

@@ -1709,7 +1709,13 @@ func BuildFinalModeAggregation(
 				// we must call deep clone in this case, to avoid sharing the arguments.
 				sumAgg := aggFunc.Clone()
 				sumAgg.Name = ast.AggFuncSum
-				sumAgg.RetTp = partial.Schema.Columns[partialCursor-1].GetType()
+				err := sumAgg.TypeInfer(sctx)


Does not need to infer type for count function here?

we can call inferType to align types in any case.

windtalker · 2021-11-23T08:27:00Z

planner/core/testdata/integration_serial_suite_out.json

@@ -1222,7 +1222,7 @@
          "StreamAgg 1.00 root  funcs:avg(Column#7, Column#8)->Column#4",
          "└─TableReader 1.00 root  data:StreamAgg",
          "  └─StreamAgg 1.00 batchCop[tiflash]  funcs:count(Column#9)->Column#7, funcs:sum(Column#10)->Column#8",
-          "    └─Projection 10000.00 batchCop[tiflash]  test.t.a, cast(test.t.a, decimal(15,4) BINARY)->Column#10",
+          "    └─Projection 10000.00 batchCop[tiflash]  test.t.a, cast(test.t.a, decimal(37,4) BINARY)->Column#10",


Why this will be changed?

it is from injectProjBlewAgg()

fzhedu · 2021-11-29T11:35:13Z

/run-check_dev_2

fzhedu · 2021-11-30T04:39:41Z

/run-check_dev_2

fzhedu · 2021-11-30T04:39:49Z

/run-check_dev

fzhedu · 2021-11-30T04:52:22Z

/run-check_dev_2

lzmhhh123 · 2021-11-30T05:56:32Z

/run-all-tests

fzhedu · 2021-11-30T06:47:47Z

/run-integration-tests

fzhedu · 2021-11-30T06:49:18Z

/run-all-tests

fzhedu · 2021-11-30T08:18:34Z

/run-integration-tests

lzmhhh123

@fzhedu After the change. The integration copr test failed. Please check it.

[2021-11-30T08:22:04.629Z] 2021/11/30 16:22:04 2021/11/30 16:21:04 Test fail: Outputs are not matching.

[2021-11-30T08:22:04.629Z] Test case: sql/randgen/10_agg.sql

[2021-11-30T08:22:04.629Z] Statement: #36 -  SELECT AVG( `col_bigint` / `col_smallint` ) AS field1 FROM `table10_int_autoinc` WHERE `col_varchar_64`  /* QNO 38 CON_ID 1137 */ ;

[2021-11-30T08:22:04.629Z] NoPushDown Output: 

[2021-11-30T08:22:04.629Z] field1

[2021-11-30T08:22:04.629Z] -101012123512698.87090000

[2021-11-30T08:22:04.629Z] 

[2021-11-30T08:22:04.629Z] 

[2021-11-30T08:22:04.629Z] WithPushDown Output: 

[2021-11-30T08:22:04.629Z] field1

[2021-11-30T08:22:04.629Z] -101012123512698.87093722

fzhedu · 2021-11-30T13:25:10Z

/run-all-tests

fzhedu · 2021-12-01T03:01:28Z

/run-check_dev_2

fzhedu · 2021-12-01T03:34:19Z

/run-integration-tests

fzhedu · 2021-12-01T03:34:41Z

@fzhedu After the change. The integration copr test failed. Please check it.

[2021-11-30T08:22:04.629Z] 2021/11/30 16:22:04 2021/11/30 16:21:04 Test fail: Outputs are not matching.

[2021-11-30T08:22:04.629Z] Test case: sql/randgen/10_agg.sql

[2021-11-30T08:22:04.629Z] Statement: #36 -  SELECT AVG( `col_bigint` / `col_smallint` ) AS field1 FROM `table10_int_autoinc` WHERE `col_varchar_64`  /* QNO 38 CON_ID 1137 */ ;

[2021-11-30T08:22:04.629Z] NoPushDown Output: 

[2021-11-30T08:22:04.629Z] field1

[2021-11-30T08:22:04.629Z] -101012123512698.87090000

[2021-11-30T08:22:04.629Z] 

[2021-11-30T08:22:04.629Z] 

[2021-11-30T08:22:04.629Z] WithPushDown Output: 

[2021-11-30T08:22:04.629Z] field1

[2021-11-30T08:22:04.629Z] -101012123512698.87093722

fixed

fzhedu · 2021-12-01T06:14:17Z

/run-idc-jenkins-ci-tidb/integration-copr-test

windtalker · 2021-12-01T06:25:43Z

executor/aggfuncs/func_avg.go

+	if e.retTp == nil {
+		return errors.New("e.retTp of avg should not be nil")
+	}
+	err = finalResult.Round(finalResult, e.retTp.Decimal, types.ModeHalfEven)


If e.retTp.Decimal == -1, before this change, the final result will be rounded with scale = MaxDecimalScale, after this change, the final result will be rounded with scale = -1, it this the expected behavior?

windtalker · 2021-12-01T06:28:07Z

expression/aggregation/base_func.go

@@ -269,6 +283,16 @@ func (a *baseFuncDesc) typeInfer4MaxMin(ctx sessionctx.Context) {
 		(a.Name != ast.AggFuncFirstRow && a.Name != ast.AggFuncMax && a.Name != ast.AggFuncMin) {
 		a.RetTp = &types.FieldType{Tp: mysql.TypeString, Flen: mysql.MaxFieldCharLength}
 	}
+	// valid the precision and scale of decimal type
+	if a.RetTp.Tp == mysql.TypeNewDecimal {
+		a.RetTp = a.RetTp.Clone()


If the input type is decimal(23,-1), then it will be changed to decimal(23,30)?

Does this take affect for all the functions? For example, currently, if we try to add implicit cast from double to decimal, the return type of the cast function will be decimal(23,-1), so after this pr, it will be updated to decimal(23,30)?

#30347 let it as another issue.