executor: support new aggregate framework for HashAggExec #7268

XuHuaiyu · 2018-08-03T05:23:44Z

What have you changed? (mandatory)

This PR supports the new aggregate framework for HashAggExec.
Since HashAggExec has supported parallel execution, we introduce
MergePartialResult to merge the partial results in the final phase,
which would avoid the cost of converting PartialResult to chunk.

Please review the following PRs before this PR:

expression, executor: add a new interface MergePartialResult for the new aggregation framework #7281
executor: fix group_concat when chunk size is set to 1 #7328

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)

exist tests

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)

#6952

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

shenli · 2018-08-05T07:23:40Z

@XuHuaiyu This PR is too big. Would you please split it into a few small ones?

XuHuaiyu · 2018-08-06T03:14:55Z

I'll split this PR into small ones:

refine aggFunc.Build to buildFinalFunc for parallel execution. expression, executor: add a new interface MergePartialResult for the new aggregation framework #7281
add AppendPartialResult2Chunk for AggFunc interface.
modify the evaluate logic using new interface for hash agg.

zz-jason · 2018-08-13T09:34:15Z

@XuHuaiyu All the dependency are satisfied, is this PR ready to be reviewed?

XuHuaiyu · 2018-08-13T09:38:01Z

yes, I'm removing the useless code. @zz-jason

XuHuaiyu · 2018-08-13T11:23:49Z

/run-all-tests

XuHuaiyu · 2018-08-15T02:51:44Z

/run-all-tests

XuHuaiyu · 2018-08-15T05:23:47Z

/run-common-test tidb-test=pr/599
/run-integration-common-test tidb-test=pr/599

winoros · 2018-08-15T07:24:28Z

executor/aggfuncs/func_group_concat.go

 		for _, arg := range e.args {
 			v, isNull, err = arg.EvalString(sctx, row)
 			if err != nil {
 				return errors.Trace(err)
 			}
 			if isNull {
-				continue
+				break


Why change here?

A bug-fix.
Test case is here. https://github.com/pingcap/tidb/pull/7268/files#diff-ab83e4a8f21f295ae805f30d7e889703R332

winoros · 2018-08-15T07:25:03Z

executor/aggregate.go

@@ -31,16 +30,16 @@ import (
 	"golang.org/x/net/context"
 )

-type aggCtxsMapper map[string][]*aggregation.AggEvaluateContext
+type aggCtxsMapper map[string][]aggfuncs.PartialResult


Do we need to change its name to something like partialResultMapper?

XuHuaiyu · 2018-08-15T08:24:02Z

PTAL @winoros @zz-jason

XuHuaiyu · 2018-08-16T03:10:58Z

PTAL @winoros @zz-jason

zz-jason · 2018-08-16T08:29:02Z

plan/rule_eliminate_projection.go

@@ -51,9 +51,9 @@ func canProjectionBeEliminatedStrict(p *PhysicalProjection) bool {
 func resolveColumnAndReplace(origin *expression.Column, replace map[string]*expression.Column) {
 	dst := replace[string(origin.HashCode(nil))]
 	if dst != nil {
-		colName := origin.ColName
+		colName, retType := origin.ColName, origin.RetType


why make this change?

e.g.

tidb> desc select distinct a from t2 ; +---------------------+----------+------+-------------------------------------------------------------+ | id | count | task | operator info | +---------------------+----------+------+-------------------------------------------------------------+ | HashAgg_4 | 8000.00 | root | group by:test.t2.a, funcs:firstrow(test.t2.a) | | └─TableReader_8 | 10000.00 | root | data:TableScan_7 | | └─TableScan_7 | 10000.00 | cop | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo | +---------------------+----------+------+-------------------------------------------------------------+ 3 rows in set (0.00 sec)

There will be a Projection between HashAgg4 and TableReader_8 during logical plan building,
whose schema is [a (type enum)].
We reset the return type of origin (which is from HashAgg4's schema) here to make sure the return type of HashAgg_4 is the result of type inferring rather than the return type of Projection.

zz-jason · 2018-08-16T08:29:59Z

plan/logical_plan_builder.go

@@ -594,6 +594,11 @@ func (b *planBuilder) buildDistinct(child LogicalPlan, length int) *LogicalAggre
 	}
 	plan4Agg.SetChildren(child)
 	plan4Agg.SetSchema(child.Schema().Clone())
+	// Distinct will be rewritten as first_row, we reset the type here since the return type
+	// of first_row is not always the same as the column arg of first_row.
+	for i, col := range plan4Agg.schema.Columns {


should we make the same change for all the places that create the schema of aggregate operator?

We reset the type of aggregate here since this is not correct to set the schema of plan4Agg as the child's schema in line 596.

If the schema of aggregate is set correctly, we do not need to make the same change.

zz-jason · 2018-08-16T08:32:20Z

expression/aggregation/descriptor.go

@@ -345,6 +345,9 @@ func (a *AggFuncDesc) typeInfer4MaxMin(ctx sessionctx.Context) {
 		a.Args[0] = expression.BuildCastFunction(ctx, a.Args[0], tp)
 	}
 	a.RetTp = a.Args[0].GetType()
+	if a.RetTp.Tp == mysql.TypeEnum || a.RetTp.Tp == mysql.TypeSet {


should we also check TypeBit?

TypeBit has been checked when buildMaxMin and buildFirstRow.
I tried to check it here, but it caused other problems,
I'll try to move the check of TypeBit here individually.

zz-jason · 2018-08-16T08:45:19Z

executor/aggregate.go

-		return rows, false
+// getPartialResultBatch fetches a batch of partial results from HashAggIntermData.
+func (d *HashAggIntermData) getPartialResultBatch(sc *stmtctx.StatementContext, prs [][]aggfuncs.PartialResult, aggFuncs []aggfuncs.AggFunc, maxChunkSize int) (_ [][]aggfuncs.PartialResult, groupKeys [][]byte, reachEnd bool) {
+	if len(prs) == maxChunkSize {


It seems that this function returns all the (group key, partial result) pairs back to the caller, this check can be removed?

zz-jason · 2018-08-16T08:49:01Z

executor/aggregate.go

@@ -73,7 +72,7 @@ type HashAggFinalWorker struct {

 	rowBuffer           []types.Datum
 	mutableRow          chunk.MutRow
-	aggCtxsMap          aggCtxsMapper
+	partialResultMap    aggPartialResultMapper
 	groupSet            *mvmap.MVMap


seems groupSet can be declared as map[string]struct{}, or just use executor.aggfuncs.stringSet?

We can leave it to the future PRs.

shenli · 2018-08-19T07:10:08Z

@XuHuaiyu Please address the comments.

XuHuaiyu · 2018-08-20T03:53:21Z

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

XuHuaiyu · 2018-08-20T04:51:43Z

/run-common-test tidb-test=pr/599
/run-integration-common-test tidb-test=pr/599

zz-jason

LGTM

XuHuaiyu · 2018-08-20T08:30:29Z

PTAL @winoros

winoros

lgtm
After this pr, if we group_concat on a sorted source. Will it still be no order?

XuHuaiyu · 2018-08-21T07:17:06Z

@winoros
For parallel hash agg, yes.
We need to implement

group_concat([ORDER BY {unsigned_integer | col_name | expr}
                 [ASC | DESC] [,col_name ...]])

to make it orderly.

XuHuaiyu added type/enhancement The issue or PR belongs to an enhancement. status/WIP sig/execution SIG execution labels Aug 3, 2018

XuHuaiyu added this to the 2.1 milestone Aug 3, 2018

XuHuaiyu force-pushed the hash_agg branch from c035735 to 6c5a17a Compare August 5, 2018 09:02

XuHuaiyu mentioned this pull request Aug 6, 2018

expression, executor: add a new interface MergePartialResult for the new aggregation framework #7281

Merged

XuHuaiyu force-pushed the hash_agg branch from 6c5a17a to 578dc00 Compare August 9, 2018 03:02

executor: use new aggregate framework for HashAggExec

3e88f00

XuHuaiyu force-pushed the hash_agg branch from 346a799 to 3e88f00 Compare August 13, 2018 11:20

XuHuaiyu removed the status/WIP label Aug 13, 2018

XuHuaiyu added 2 commits August 14, 2018 10:53

fix bug for first_row/max/min(enum/set)

26c6b16

fix bug when agg funcs is empty or/and groupByItems is empty

011e597

XuHuaiyu added the priority/P1 The issue has P1 priority. label Aug 15, 2018

fix group_concat bug when arg contains null

9e1f1ae

winoros reviewed Aug 15, 2018

View reviewed changes

addess comment

be45b8b

XuHuaiyu added the status/all tests passed label Aug 15, 2018

zz-jason reviewed Aug 16, 2018

View reviewed changes

XuHuaiyu added 2 commits August 20, 2018 11:50

address comment

f4b29f3

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

c013738

zz-jason reviewed Aug 20, 2018

View reviewed changes

shenli added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 20, 2018

winoros approved these changes Aug 20, 2018

View reviewed changes

Merge branch 'master' into hash_agg

d229be6

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 21, 2018

XuHuaiyu merged commit 4836aa3 into pingcap:master Aug 21, 2018

XuHuaiyu mentioned this pull request Aug 21, 2018

Tune the perfomance of hash aggregate operator #7450

Closed

tiancaiamao mentioned this pull request Sep 3, 2018

slice bounds out of range #7578

Closed

XuHuaiyu deleted the hash_agg branch December 12, 2018 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support new aggregate framework for HashAggExec #7268

executor: support new aggregate framework for HashAggExec #7268

XuHuaiyu commented Aug 3, 2018 •

edited

Loading

shenli commented Aug 5, 2018

XuHuaiyu commented Aug 6, 2018 •

edited

Loading

zz-jason commented Aug 13, 2018

XuHuaiyu commented Aug 13, 2018

XuHuaiyu commented Aug 13, 2018

XuHuaiyu commented Aug 15, 2018

XuHuaiyu commented Aug 15, 2018

winoros Aug 15, 2018

XuHuaiyu Aug 15, 2018

winoros Aug 15, 2018

XuHuaiyu commented Aug 15, 2018

XuHuaiyu commented Aug 16, 2018

zz-jason Aug 16, 2018

XuHuaiyu Aug 20, 2018

zz-jason Aug 16, 2018

XuHuaiyu Aug 20, 2018

zz-jason Aug 16, 2018

XuHuaiyu Aug 20, 2018

zz-jason Aug 16, 2018 •

edited

Loading

zz-jason Aug 16, 2018

shenli commented Aug 19, 2018

XuHuaiyu commented Aug 20, 2018

XuHuaiyu commented Aug 20, 2018

zz-jason left a comment

XuHuaiyu commented Aug 20, 2018

winoros left a comment

XuHuaiyu commented Aug 21, 2018

executor: support new aggregate framework for HashAggExec #7268

executor: support new aggregate framework for HashAggExec #7268

Conversation

XuHuaiyu commented Aug 3, 2018 • edited Loading

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)

shenli commented Aug 5, 2018

XuHuaiyu commented Aug 6, 2018 • edited Loading

zz-jason commented Aug 13, 2018

XuHuaiyu commented Aug 13, 2018

XuHuaiyu commented Aug 13, 2018

XuHuaiyu commented Aug 15, 2018

XuHuaiyu commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Aug 15, 2018

XuHuaiyu commented Aug 16, 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 Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Aug 19, 2018

XuHuaiyu commented Aug 20, 2018

XuHuaiyu commented Aug 20, 2018

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Aug 20, 2018

winoros left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Aug 21, 2018

XuHuaiyu commented Aug 3, 2018 •

edited

Loading

XuHuaiyu commented Aug 6, 2018 •

edited

Loading

zz-jason Aug 16, 2018 •

edited

Loading