planner/core: enhance the rule of group pruning #9431

bb7133 · 2019-02-24T09:10:40Z

What problem does this PR solve?

This PR aims to implement the rule described in #7700, in current 'core' planner

What is changed and how it works?

A new tryToEliminateAggregationByMapping() is added, it does the following things:

Find a nested LogicalAggregation plan pattern from group by query with another group by subquery. Let's call them outter and inner aggregations, respectively.
Tries to map schema of outter aggregation to the schema of inner aggregation, so that group-by expressions/aggregated functions can be compared.
Check the semantic of all aggregated functions to determine if one of the aggregations can be eliminated.

Tests

Integration test

Side effects

Possible performance regression
Increased code complexity

codecov-io · 2019-02-24T14:33:55Z

Codecov Report

Merging #9431 into master will decrease coverage by 0.0579%.
The diff coverage is 82.0809%.

@@               Coverage Diff               @@
##             master      #9431       +/-   ##
===============================================
- Coverage   79.9286%   79.8707%   -0.058%     
===============================================
  Files           460        460               
  Lines        102878     102582      -296     
===============================================
- Hits          82229      81933      -296     
- Misses        14688      14709       +21     
+ Partials       5961       5940       -21

alivxxx · 2019-02-26T02:54:29Z

planner/core/rule_aggregation_elimination.go

-// tryToEliminateAggregation will eliminate aggregation grouped by unique key.
+// nestedAggPattern stores nested LogicalAggregations, so they can be accessed easily
+type nestedAggPattern struct {
+	outter *LogicalAggregation


fixed, I checked some other spelling typos as well, thanks

alivxxx · 2019-02-26T02:54:55Z

planner/core/rule_aggregation_elimination.go

@@ -31,11 +31,20 @@ type aggregationEliminator struct {
 type aggregationEliminateChecker struct {
 }

-// tryToEliminateAggregation will eliminate aggregation grouped by unique key.
+// nestedAggPattern stores nested LogicalAggregations, so they can be accessed easily


Add . at the end of sentences.

alivxxx · 2019-02-26T02:55:01Z

planner/core/rule_aggregation_elimination.go

+	outter *LogicalAggregation
+	proj   *LogicalProjection
+	inner  *LogicalAggregation
+	// isTrivial indicates if there's constraints(like LogicalSelection/LogicalLimit) "between" them


alivxxx · 2019-02-26T05:39:35Z

planner/core/rule_aggregation_elimination.go

+}
+
+// genColMaps generates exprMap(`col -> definition expr`) and aggMap(`col -> definition aggFunc`) from column definitions
+func genColMaps(ctx sessionctx.Context, ptn *nestedAggPattern) (map[string]expression.Expression, map[string]*aggregation.AggFuncDesc) {


ctx is not used.

alivxxx · 2019-02-26T05:47:10Z

planner/core/rule_aggregation_elimination.go

+func genColMaps(ctx sessionctx.Context, ptn *nestedAggPattern) (map[string]expression.Expression, map[string]*aggregation.AggFuncDesc) {
+	exprMap := make(map[string]expression.Expression, len(ptn.proj.Schema().Columns))
+	for _, col := range ptn.proj.Schema().Columns {
+		idx := ptn.proj.Schema().ColumnIndex(col)


Is it not the offset in the array?

alivxxx · 2019-02-26T08:00:22Z

planner/core/rule_aggregation_elimination.go

+	var newExprs []expression.Expression
+	replaced := false
+	for idx, expr := range exprs {
+		if e, ok := exprMap[string(expr.HashCode(nil))]; ok {


Why pass nil? Will it cause panic?

fixed, I added a test case as well

alivxxx · 2019-02-26T08:00:32Z

planner/core/rule_aggregation_elimination.go

+	var newExprs []expression.Expression
+	replaced := false
+	for idx, expr := range exprs {
+		if fun, ok := aggMap[string(expr.HashCode(nil))]; ok {


bb7133 · 2019-03-01T11:36:38Z

hi @lamxTyler , addressed, thanks for the comments!

bb7133 · 2019-04-01T12:23:29Z

PTAL @eurekaka , thanks!

eurekaka · 2019-04-03T05:41:32Z

planner/core/rule_aggregation_elimination.go

+	outer *LogicalAggregation
+	proj  *LogicalProjection
+	inner *LogicalAggregation
+	// isTrivial indicates if there's constraints(like LogicalSelection/LogicalLimit) "between" them.


Suggested change

// isTrivial indicates if there's constraints(like LogicalSelection/LogicalLimit) "between" them.

// isTrivial indicates if there are operators(like LogicalSelection/LogicalLimit) between 2 aggs.

eurekaka · 2019-04-03T06:06:25Z

planner/core/rule_aggregation_elimination.go

@@ -136,6 +145,114 @@ func (a *aggregationEliminateChecker) wrapCastFunction(ctx sessionctx.Context, a
 	return expression.BuildCastFunction(ctx, arg, targetTp)
 }

+// tryToEliminateAggregationByMapping tries to eliminate an aggregation from nested aggregations.


Actually we are eliminating the nested aggregation? Please make this comment clearer.

eurekaka · 2019-04-03T06:09:12Z

planner/core/rule_aggregation_elimination.go

+//  can be rewritten as
+//  `select a + b as p, count(d) as `max(dt)` from t group by a + b`
+//
+// 2. Nested aggregations in which outer group-by items are proper subset of the inner ones, for example:


Suggested change

// 2. Nested aggregations in which outer group-by items are proper subset of the inner ones, for example:

// 2. Group by items of outer aggregation are super set of nested aggregation, for example:

Addressed, thanks

eurekaka · 2019-04-03T06:10:22Z

planner/core/rule_aggregation_elimination.go

+//  can be rewritten as
+//  `select a as at, max(b) as `max(bt)` from t group by a`
+//
+// In order to apply the optimization, we tries to map definition of outer aggregation to schema of inner aggregation and check if rules are met.


Suggested change

// In order to apply the optimization, we tries to map definition of outer aggregation to schema of inner aggregation and check if rules are met.

// We map definition of outer aggregation to schema of inner aggregation and check if the rules can be applied.

Addressed, thanks

eurekaka · 2019-04-03T06:15:04Z

planner/core/rule_aggregation_elimination.go

+	// Map group-by items in outer aggregation to the schema of inner aggregation, so the group-by items can be compared.
+	_, items := exprSubstitute(la.ctx, exprMap, ptn.outer.GroupByItems)
+	_, items = aggSubstitute(la.ctx, aggMap, items, func(fun *aggregation.AggFuncDesc) bool {
+		// Outer group-by items cannot be aggregated result of inner aggregation.


We don't need this check? the "group-by subset check" has already guarantees this?

The check here is to prevent the following case:

select bt, max(ct) from (select b as bt, count(c) as ct from t group by c, b) tt group by ct;

ct is defined as count(c) and it is tricky to indicate the semantics of aggregations here.

eurekaka · 2019-04-03T07:56:23Z

planner/core/rule_aggregation_elimination.go

+	newFuns := make([]*aggregation.AggFuncDesc, len(ptn.outer.AggFuncs))
+	for idx, fun := range ptn.outer.AggFuncs {
+		_, exprs := exprSubstitute(la.ctx, exprMap, fun.Args)
+		expr := exprs[0]


Add a check for single-param aggregation?

eurekaka · 2019-04-03T08:03:38Z

planner/core/rule_aggregation_elimination.go

+	return false, exprs
+}
+
+func deepContains(ctx sessionctx.Context, exprs []expression.Expression, expr expression.Expression) bool {


How about renaming it to EqualContains and move it to be with Contains in expression/util.go?

eurekaka · 2019-04-03T08:05:06Z

planner/core/rule_aggregation_elimination.go

+			if subItem.Equal(la.ctx, item) {
+				found = true
+				break
+			}


Can we use deepContains here?

Addressed, thanks!

eurekaka · 2019-04-03T08:35:05Z

planner/core/rule_aggregation_elimination.go

+	combined.Args[0] = expr
+
+	if innerIsGroup {
+		if (inner.Name == ast.AggFuncFirstRow || inner.Name == ast.AggFuncMax || inner.Name == ast.AggFuncMin) &&


Could you explain the rationale behind this condition? From my understanding, if innerIsGroup is true, we can just keep the outer aggregation without change, no matter what kind of aggregation it is?

If the item being aggregated in outter aggregation is group-by item of inner aggregation. We can only do elimination on very few conditions.
For example, given

select sum(at) from (select count(a) as at, b as bt from t group by a, b) as t group by bt

AggFunc count(a) is always 1 and sum(at) is distinct number of a. We only consider the simplest case here because for other cases, there is no common rule to indicate the semantics.

eurekaka · 2019-04-03T08:51:12Z

planner/core/rule_aggregation_elimination.go

+//  * true / substituted exprs if the substitution happens.
+//  * false / original exprs if substitution doesn't happen.
+//  * false / nil if predicate is not nil and it failed.
+func aggSubstitute(


This function shares a lot of logic with exprSubstitute, is it possible to merge them together? or using ColumnSubstitute instead?

Done, thanks

eurekaka · 2019-04-04T02:24:59Z

planner/core/rule_aggregation_elimination.go

+	case inner.Name == ast.AggFuncBitOr && outer.Name == ast.AggFuncBitOr:
+	case inner.Name == ast.AggFuncBitXor && outer.Name == ast.AggFuncBitXor:
+	case inner.Name == ast.AggFuncGroupConcat && outer.Name == ast.AggFuncGroupConcat:
+		if inner.HasDistinct {


first_row / min / max / BitAnd / BitOr / BitXor is applicable even if inner.HasDistinct is true? Also, group_concat cannot be simply merged because it may contain ORDER BY and SEPARATOR keyword inside the group_concat?

first_row, min, max, bit_and, bit_or accept HasDistinct because they're idempotent. But bit_xor is not, I've fixed it, thanks!

zz-jason · 2019-08-03T08:21:33Z

@bb7133 friendly ping, any update?

bb7133 · 2019-08-05T01:22:31Z

I will update the code this week @zz-jason

Thanks for the reviews @eurekaka

bb7133 · 2019-08-14T12:27:36Z

Still working on this PR

bb7133 · 2019-08-23T08:27:33Z

Close this PR temporarily because of the PR quota.

bb7133 · 2019-09-04T10:06:02Z

/run-all-tests

francis0407 · 2019-09-23T06:20:30Z

planner/core/logical_plan_test.go

@@ -1647,6 +1647,126 @@ func (s *testPlanSuite) TestAggPrune(c *C) {
 			sql:  "select a, count(distinct a, b) from t group by a",
 			best: "DataScan(t)->Projection",
 		},
+		{


We'd better move these tests to json file now.

Do you mean the explaintest?

Not explain test, you can refer to #12091

Addressed, PTAL

francis0407 · 2019-09-23T06:51:25Z

planner/core/rule_aggregation_elimination.go

+		}
+		// Generalize isGbItem: aggregate function can also be treated as group by item.
+		// For example, select max(at), sum(bt) from (select a as at, count(b) as bt from t group by a) as tt,
+		// in such case column `at` is an alias of `max(a)`,  and `max(a)` is equivalent of `a`, which is the group by


column at is an alias of max(a), and max(a) is equivalent of a

Why is at an alias of max(a) ?

This is a typo, I've fixed the comment as following:

... // For example, select max(at), sum(bt) from (select max(a) as at, count(b) as bt from t group by a) as tt, // in such case column `at` is an alias of `max(a)`, and `max(a)` is equivalent of `a`(`firstrow(a)`), which is the group by // item of inner aggregation.

Thanks.

francis0407 · 2019-09-23T06:53:43Z

planner/core/rule_aggregation_elimination.go

+
+// tryToCombineAggFunc checks the types of inner/outer aggregate function and check if they can be combined as one based on their semantics.
+// for example, since max(max(PARTIAL)) can be combined as max(TOTAL), we can combine inner max() and outer max() as a final max()
+// `innerIsGroup` indicates if the inner aggregate function is aggregating group-by items(values are de-duplicated).


innerIsGroup -> innerIsGbItem?

francis0407 · 2019-09-23T06:57:55Z

planner/core/rule_aggregation_elimination.go

+		// Generalize isGbItem: aggregate function can also be treated as group by item.
+		// For example, select max(at), sum(bt) from (select a as at, count(b) as bt from t group by a) as tt,
+		// in such case column `at` is an alias of `max(a)`,  and `max(a)` is equivalent of `a`, which is the group by
+		// item of inner aggregation


add . at the end of the comment.

francis0407 · 2019-09-23T07:14:26Z

planner/core/rule_aggregation_elimination.go

+		return nil
+	}
+
+	newFuns := make([]*aggregation.AggFuncDesc, len(ptn.outer.AggFuncs))


newFuncs seems better?

sre-bot · 2019-09-25T07:29:14Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

sre-bot · 2019-09-27T07:29:59Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

sre-bot · 2019-10-01T13:11:35Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

1. fix some spelling typo 2. fix expr.HashCode(nil) bug 3. refined some other code styles

bb7133 · 2019-10-03T08:23:57Z

Hi @francis0407 , thanks for the reviews, PTAL

sre-bot · 2019-10-05T08:23:36Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

sre-bot · 2019-10-07T08:24:15Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

sre-bot · 2019-10-10T08:25:02Z

@lamxTyler, @eurekaka, @lzmhhh123, PTAL.

bb7133 force-pushed the bb7133/enhance_agg_elim branch from acf5bc9 to 17ea4ef Compare February 24, 2019 14:21

morgo added contribution This PR is from a community contributor. sig/planner SIG: Planner labels Feb 24, 2019

zz-jason requested review from eurekaka and alivxxx February 25, 2019 08:27

alivxxx reviewed Feb 26, 2019

View reviewed changes

bb7133 force-pushed the bb7133/enhance_agg_elim branch from 17ea4ef to 387ccd9 Compare March 1, 2019 11:19

bb7133 force-pushed the bb7133/enhance_agg_elim branch from 69659ab to ee61818 Compare April 1, 2019 12:06

eurekaka added the type/enhancement The issue or PR belongs to an enhancement. label Apr 3, 2019

eurekaka reviewed Apr 3, 2019

View reviewed changes

eurekaka reviewed Apr 4, 2019

View reviewed changes

lzmhhh123 self-requested a review July 31, 2019 09:58

zz-jason added the status/ReqChange label Aug 3, 2019

sre-bot mentioned this pull request Aug 23, 2019

main: add make coverage_server build option #11844

Merged

bb7133 closed this Aug 23, 2019

bb7133 reopened this Aug 31, 2019

bb7133 force-pushed the bb7133/enhance_agg_elim branch 3 times, most recently from 3826745 to 60f6568 Compare September 4, 2019 09:40

francis0407 self-requested a review September 9, 2019 04:58

bb7133 force-pushed the bb7133/enhance_agg_elim branch from a51548b to 5522bfd Compare September 12, 2019 14:15

francis0407 reviewed Sep 23, 2019

View reviewed changes

zz-jason removed the contribution This PR is from a community contributor. label Sep 25, 2019

bb7133 added 6 commits October 2, 2019 23:48

planner/core: enhance the rule of group pruning

adbed3a

address lamxTyler's comments

8b0aaa9

1. fix some spelling typo 2. fix expr.HashCode(nil) bug 3. refined some other code styles

fix merge conflict

ba6e052

address eurekaka's comments

1d40f0a

address eurekaka's comments

69777fc

address francis0407's comments

e2ce80d

bb7133 force-pushed the bb7133/enhance_agg_elim branch from 6f2788c to e2ce80d Compare October 3, 2019 08:19

Merge branch 'master' into bb7133/enhance_agg_elim

32c325c

bb7133 requested review from qw4990, Reminiscent, SunRunAway and XuHuaiyu as code owners October 8, 2019 06:14

SunRunAway removed their request for review October 8, 2019 07:32

XuHuaiyu removed their request for review October 8, 2019 08:41

qw4990 removed their request for review October 9, 2019 08:47

Reminiscent removed their request for review October 9, 2019 09:01

lzmhhh123 removed their request for review October 14, 2019 11:06

This was referenced Nov 6, 2019

ddl: make TestModifyColumnNullToNotNull test stable #13223

Merged

infoschema: add TIDB_ROW_ID_SHARDING_INFO for table table. #13342

Closed

github-actions bot added the component/expression label Mar 8, 2020

bb7133 closed this Sep 2, 2020

	// isTrivial indicates if there's constraints(like LogicalSelection/LogicalLimit) "between" them.
	// isTrivial indicates if there are operators(like LogicalSelection/LogicalLimit) between 2 aggs.

	// 2. Nested aggregations in which outer group-by items are proper subset of the inner ones, for example:
	// 2. Group by items of outer aggregation are super set of nested aggregation, for example:

	// In order to apply the optimization, we tries to map definition of outer aggregation to schema of inner aggregation and check if rules are met.
	// We map definition of outer aggregation to schema of inner aggregation and check if the rules can be applied.

planner/core: enhance the rule of group pruning #9431

planner/core: enhance the rule of group pruning #9431

Conversation

bb7133 commented Feb 24, 2019

What problem does this PR solve?

What is changed and how it works?

codecov-io commented Feb 24, 2019 • edited by codecov bot Loading

Codecov Report

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

bb7133 commented Mar 1, 2019

bb7133 commented Apr 1, 2019

Choose a reason for hiding this comment

bb7133 Aug 31, 2019 • 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

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 Aug 3, 2019

bb7133 commented Aug 5, 2019

bb7133 commented Aug 14, 2019

bb7133 commented Aug 23, 2019

bb7133 commented Sep 4, 2019

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

sre-bot commented Sep 25, 2019

sre-bot commented Sep 27, 2019

sre-bot commented Oct 1, 2019

bb7133 commented Oct 3, 2019

sre-bot commented Oct 5, 2019

sre-bot commented Oct 7, 2019

sre-bot commented Oct 10, 2019

codecov-io commented Feb 24, 2019 •

edited by codecov bot

Loading

bb7133 Aug 31, 2019 •

edited

Loading