add sum_decimal method for aggfunc #7096

laidahe · 2018-07-18T12:26:55Z

add sum_decimal method for aggfunc

What have you changed? (mandatory)

partially implement the SUM function under the new aggregate function framework, only implemented the following SUM functions:

SUM(DECIMAL)
the following SUM functions are not implemented:
SUM (DISTINCT DECIMAL)
SUM(DISTINCT FLOAT64)
SUM (FLOAT64)

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

6952

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

add sum_decimal of aggfunc

sre-bot · 2018-07-18T12:26:58Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

CLAassistant · 2018-07-18T12:27:03Z

All committers have signed the CLA.

zz-jason · 2018-07-18T12:37:02Z

executor/aggfuncs/func_sum_decimal.go

+
+type partialResult4SumDecimal struct {
+	sum   types.MyDecimal
+	count int64


maybe we could use a bool value isNull to indicate whether there exists a sum value in the partial result?

please use isNull instead, you can take the MAX/MIN for example

but i can not find the file func_max.go in master.

zz-jason · 2018-07-19T00:10:51Z

executor/aggfuncs/func_sum_decimal.go

+}
+
+
+type sumPartial4Decimal struct {


Probably there is no need to implement the sumPartial4Decimal? Because no matter what mode is the sum aggregate function stays in, it always has one parameter and the type is decimal.

yep,i find out that the logic of sumPartial4Decimal and sumOriginal4Decimal are the same.
Would you give me some suggestion on renaming sumOriginal4Decimal?

zz-jason · 2018-07-19T00:12:10Z

executor/aggfuncs/func_sum_decimal.go

@@ -0,0 +1,127 @@
+// Copyright 2018 PingCAP, Inc.


We'd better name this file to func_sum.go to follow the same file naming style with other aggregate functions.

Is it better to separate SUM(decimal) and SUM(float) to single file?

I think it's better to follow the existing flavors. Not only aggregate functions, but also other components, for example, expression and executors, they all put the code with same functionality in the same source file. 😄

ok,follow the existing flavors

please rename this file to func_sum.go

zz-jason · 2018-07-19T00:16:24Z

executor/aggfuncs/func_sum_decimal.go

+	return nil
+}
+
+type sumOriginal4Decimal struct {


It would be nice if you add this implementation to the aggfuncs.go for code navigating, you can take other aggregate functions listed in aggfuncs.go as examples 😄

go fmt the file

zz-jason · 2018-07-21T02:51:26Z

@laidahe please add build functions for SUM, and add the implementations to aggfuncs.go

support int / uint / decimal for sum

XuHuaiyu

executor/ aggfuncs/ builder.go
buildSum also should be implemented.

XuHuaiyu · 2018-07-23T06:29:35Z

executor/aggfuncs/func_sum.go

+	"github.com/pingcap/tidb/util/chunk"
+)
+
+// All the following sum function implementations return the decimal result,


Why put the comment here?

i delete them

XuHuaiyu · 2018-07-23T06:36:13Z

executor/aggfuncs/func_sum.go

+
+func (e *sumAggFunc4Decimal) AllocPartialResult() PartialResult {
+	p := new(partialResult4SumDecimal)
+	//p.val = *types.NewDecFromInt(0) should the line un-commented?


This line can be removed.

XuHuaiyu · 2018-07-23T06:36:19Z

executor/aggfuncs/func_sum.go

+
+func (e *sumAggFunc4Decimal) ResetPartialResult(pr PartialResult) {
+	p := (*partialResult4SumDecimal)(pr)
+	//p.val = *types.NewDecFromInt(0)   should the line un-commented?


XuHuaiyu · 2018-07-23T06:38:51Z

executor/aggfuncs/func_sum.go

+// All the following sum function implementations return the decimal result,
+// which store the partial results in "partialResult4SumDecimal".
+
+type partialResult4SumInt struct {


We do not implement partialResult4SumInt/partialResult4SumUint.
We only need partialResult4SumDecimal and partialResult4SumFloat64.

MySQL only returns type double or type decimal.

laidahe · 2018-07-24T07:21:14Z

PTAL

zz-jason · 2018-07-24T07:25:56Z

@laidahe Please merge master and fix CI

zz-jason · 2018-07-25T12:22:59Z

executor/aggfuncs/builder.go

@@ -112,8 +137,8 @@ func buildAvg(aggFuncDesc *aggregation.AggFuncDesc, ordinal int) AggFunc {
 	case aggregation.DedupMode:
 		return nil // not implemented yet.

-	// Build avg functions which consume the original data and update their
-	// partial results.
+		// Build avg functions which consume the original data and update their


why make this change?

maybe the editor mistake.i change it back.

zz-jason · 2018-07-25T12:25:10Z

executor/aggfuncs/func_sum.go

+func (e *sum4Float64) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
+	p := (*partialResult4SumFloat64)(pr)
+	for _, row := range rowsInGroup {
+		input, isNull, err := e.args[0].EvalReal(sctx, row)


EvalReal(sctx, &row) can gain better performance.

zz-jason · 2018-07-25T12:25:59Z

executor/aggfuncs/func_sum.go

+	p := (*partialResult4SumDecimal)(pr)
+	newSum := new(types.MyDecimal)
+	for _, row := range rowsInGroup {
+		input, isNull, err := e.args[0].EvalDecimal(sctx, row)


zz-jason · 2018-07-25T12:26:17Z

executor/aggfuncs/func_sum.go

+func (e *sum4DistinctFloat64) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
+	p := (*partialResult4SumDistinctFloat64)(pr)
+	for _, row := range rowsInGroup {
+		input, isNull, err := e.args[0].EvalReal(sctx, row)


zz-jason · 2018-07-25T12:26:58Z

executor/aggfuncs/func_sum.go

+	p := (*partialResult4SumDistinctDecimal)(pr)
+	var newSum types.MyDecimal
+	for _, row := range rowsInGroup {
+		input, isNull, err := e.args[0].EvalDecimal(sctx, row)


zz-jason · 2018-07-25T12:29:18Z

@laidahe you can fix ci by make dev and check the result in you computer.

zz-jason · 2018-07-25T12:31:07Z

executor/aggfuncs/aggfuncs.go

@@ -55,6 +55,9 @@ var (
 	_ AggFunc = (*avgPartial4Float64)(nil)
 	_ AggFunc = (*avgOriginal4DistinctFloat64)(nil)

+	_ AggFunc = (*sumAggFunc4Float64)(nil)


_ AggFunc = (*sum4DistinctFloat64)(nil) _ AggFunc = (*sum4DistinctDecimal)(nil) _ AggFunc = (*sum4Decimal)(nil) _ AggFunc = (*sum4Float64)(nil)

zz-jason · 2018-07-26T01:46:21Z

@laidahe CI stil fails. please fix CI. you can run make dev or make check to find the failed cases.

zz-jason · 2018-07-26T03:08:42Z

/run-all-tests

zz-jason

LGTM

XuHuaiyu

LGTM

add sum_decimal of aggfunc

7a4b2e6

add sum_decimal of aggfunc

zz-jason reviewed Jul 19, 2018

View reviewed changes

zz-jason added type/enhancement The issue or PR belongs to an enhancement. contribution This PR is from a community contributor. sig/execution SIG execution labels Jul 19, 2018

zz-jason added this to the 2.1 milestone Jul 19, 2018

go fmt the file

808a2b3

go fmt the file

laidahe added 3 commits July 21, 2018 11:38

rename func_sum_decimal to func_sum

21107c7

executor: add sum aggfunc

379ffdc

support int / uint / decimal for sum

executor: add sum aggfunc

cbfdba4

support int / uint / decimal for sum

XuHuaiyu reviewed Jul 23, 2018

View reviewed changes

add sum float64 and delete sum int/uint

3d2daca

laidahe force-pushed the master branch from be40ec8 to 3d2daca Compare July 24, 2018 03:27

add buildSum

afa3282

laidahe added 3 commits July 24, 2018 17:57

Merge branch 'master' into master

684f040

complete buildSum

d54cf79

add sum of distinct float64/decimal

7eb11eb

zz-jason reviewed Jul 25, 2018

View reviewed changes

again for ci

5112f6b

fix ci

562ed1d

Merge branch 'master' into master

1a47f81

zz-jason approved these changes Jul 26, 2018

View reviewed changes

XuHuaiyu approved these changes Jul 26, 2018

View reviewed changes

XuHuaiyu merged commit 1562f25 into pingcap:master Jul 26, 2018

add sum_decimal method for aggfunc #7096

add sum_decimal method for aggfunc #7096

Conversation

laidahe commented Jul 18, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot commented Jul 18, 2018

CLAassistant commented Jul 18, 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

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

zz-jason commented Jul 21, 2018

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

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

laidahe commented Jul 24, 2018

zz-jason commented Jul 24, 2018

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

zz-jason commented Jul 25, 2018

Choose a reason for hiding this comment

zz-jason commented Jul 26, 2018

zz-jason commented Jul 26, 2018

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 18, 2018 •

edited

Loading