planner, expression: support multi-distinct agg under MPP mode #39973

AilinKid · 2022-12-15T09:55:25Z

Signed-off-by: AilinKid [email protected]

What problem does this PR solve?

Issue Number: close #16581 #34704

What is changed and how it works?

1: implement grouping sets basic expression logic
2: implement multi-distinct agg's admission logic and 3-phase plan adjustment
3: implement a mock repeat source executor to help understand what's going on and even for test convenience.
4: implement the theoretically computed stats for partial agg, projection, and exchanger related. (details commented below)

tipb requirement: pingcap/tipb#283

tiflash optional: pingcap/tiflash#6545

For review friendly, some isolated parts can be extracted as alternative small ones. You can pick friendly here first.

Demo

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

Please refer to Release Notes Language Style Guide to write a quality release note.

support multi-distinct agg under MPP mode

ti-chi-bot · 2022-12-15T09:55:26Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

fixdb
winoros

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.

expression/grouping_sets_test.go

expression/grouping_sets.go

planner/core/explain.go

AilinKid · 2022-12-18T08:42:39Z

add another commit to adjust the stats estimated
cases diff

 [    └─HashAgg_28 1.00 mpp[tiflash]  funcs:sum(Column#20)->Column#6, funcs:sum(Column#22)->Column#7, funcs:sum(Column#24)->Column#8]
-[      └─ExchangeReceiver_36 1.00 mpp[tiflash]  ]
-[        └─ExchangeSender_35 1.00 mpp[tiflash]  ExchangeType: PassThrough]
-[          └─HashAgg_30 1.00 mpp[tiflash]  funcs:count(distinct Column#21)->Column#20, funcs:count(distinct Column#23)->Column#22, funcs:sum(Column#17)->Column#24]
-[            └─Projection_32 1.00 mpp[tiflash]  Column#17, test.t.a, test.t.b, Column#18, case(eq(Column#18, 1), test.t.a, <nil>)->Column#21, case(eq(Column#18, 2), test.t.b, <nil>)->Column#23]
-[              └─ExchangeReceiver_34 1.00 mpp[tiflash]  ]
-[                └─ExchangeSender_33 1.00 mpp[tiflash]  ExchangeType: HashPartition, Hash Cols: [name: test.t.a, collate: binary], [name: test.t.b, collate: binary], [name: Column#18, collate: binary]]
-[                  └─HashAgg_26 1.00 mpp[tiflash]  group by:Column#18, test.t.a, test.t.b, funcs:count(Column#19)->Column#17]
+[      └─ExchangeReceiver_36 1.00 mpp[tiflash]  ]
+[        └─ExchangeSender_35 1.00 mpp[tiflash]  ExchangeType: PassThrough]
+[          └─HashAgg_30 1.00 mpp[tiflash]  funcs:count(distinct Column#21)->Column#20, funcs:count(distinct Column#23)->Column#22, funcs:sum(Column#17)->Column#24]
+[            └─Projection_32 16000.00 mpp[tiflash]  Column#17, test.t.a, test.t.b, Column#18, case(eq(Column#18, 1), test.t.a, <nil>)->Column#21, case(eq(Column#18, 2), test.t.b, <nil>)->Column#23]
+[              └─ExchangeReceiver_34 16000.00 mpp[tiflash]  ]
+[                └─ExchangeSender_33 16000.00 mpp[tiflash]  ExchangeType: HashPartition, Hash Cols: [name: test.t.a, collate: binary], [name: test.t.b, collate: binary], [name: Column#18, collate: binary]]
+[                  └─HashAgg_26 16000.00 mpp[tiflash]  group by:Column#18, test.t.a, test.t.b, funcs:count(Column#19)->Column#17]
 [                    └─Projection_31 20000.00 mpp[tiflash]  test.t.a, test.t.b, test.t.c, Column#18, case(eq(Column#18, 1), test.t.c, <nil>)->Column#19]

when split mppScalar agg into as follows:
output operator <= 3 stages agg + repeat source <= input operator
the stats before input operator and after output operator (themself included) is unchanged.

what we need to do is to adjust the partial agg, middle proj, partial proj, and the exchanger between them. some notes here:

repeat source doesn't change the old input operator's NDV, it just expanded the old rows.
use the unified and theoretical stats after group items are applied over repeated data for all operators between repeat op and middle-agg.
for middle-agg and final-agg, they just group all the data in its node into one row, so use their old stats, the case here is 1

expression/aggregation/descriptor.go

expression/constant.go

go.mod

planner/core/task.go

winoros · 2022-12-27T11:50:57Z

planner/core/task.go

+				return false, nil
+			}
+			for _, arg := range fun.Args {
+				// bail out when args are not simple column, see GitHub issue #35417


How does that issue affect this branch?

I copied from hasheng's pr in single distinct-agg. @fixdb have some memory about this?

In single distinct agg's case, if the distinct key is not a simple column, planner will generate a shuffle with empty key. But In this PR, @AilinKid will project a column for complex expression, so I think the comment doesn't apply here if the projection is done before optimization.

planner/core/task.go

AilinKid · 2022-12-28T11:59:34Z

/run-unit-test

expression/constant.go

planner/core/task.go

elsa0520 · 2022-12-29T09:04:33Z

expression/grouping_sets.go

+		// for every group set，try to find its position to insert if possible，otherwise create a new grouping set.
+		for j := len(oneNewGroupingSet) - 1; j >= 0; j-- {
+			cur := oneNewGroupingSet[j]
+			if targetOne.SubSetOf(cur) {


If the targetOne is super set of cur and j=0, then it will create the new GroupingSet at the line:126.
For example:

targetOne is [a,b] gss is [ [[a],] ]

The result will be

gss [ [[a]] [[a,b]] ]

The result is not expected by your comment. Are you sure this logic is correctly ?

great catch, fixed~

Fixed, LGTM

expression/grouping_sets_test.go

planner/core/task.go

elsa0520 · 2022-12-29T09:41:14Z

planner/core/task.go

+
+// canUse3Stage4MultiDistinctAgg returns true if this agg can use 3 stage for multi distinct aggregation
+func (p *basePhysicalAgg) canUse3Stage4MultiDistinctAgg() (can bool, gss expression.GroupingSets) {
+	if !p.ctx.GetSessionVars().Enable3StageDistinctAgg || len(p.GroupByItems) > 0 {


Why we don't support multi distinct with group by ?

in the next step if needed

elsa0520 · 2022-12-29T09:47:33Z

planner/core/task.go

 	return partialAgg, finalAgg
 }

+func (p *basePhysicalAgg) scale3StageForDistinctAgg() (bool, expression.GroupingSets) {
+	if p.canUse3Stage4SingleDistinctAgg() {


Maybe this logic is more clearly ?

if (!Enable3StageDistinctAgg) { return false } distinct_num = 0; if (distinct_num == 1) { canUse3Stage4SingleDistinctAgg(); } else { canUse3Stage4MultiDistinctAgg(); }

(BTW, this is just my personal opinion)

yes, switch judgment can be taken out here, while the distinct_num can only be scratched inside

expression/aggregation/descriptor.go

fixdb · 2022-12-31T04:47:09Z

planner/core/task.go

+				return false, nil
+			}
+			for _, arg := range fun.Args {
+				// bail out when args are not simple column, see GitHub issue #35417


In single distinct agg's case, if the distinct key is not a simple column, planner will generate a shuffle with empty key. But In this PR, @AilinKid will project a column for complex expression, so I think the comment doesn't apply here if the projection is done before optimization.

fixdb · 2022-12-31T04:54:52Z

planner/core/task.go

+			// groupingID now is the offset of target grouping in GroupingSets.
+			// todo: it may be changed after grouping set merge in the future.
+			fun.GroupingID = int64(len(GroupingSets))
+		} else if len(fun.Args) > 1 {


Do you really want to bail out here? I see there are a lot of code dealing with group with multiple columns in GroupingSet.

this bailout cases like normal agg with multi args:
sum(a+b) has only one arg. --- scalarFunc(a,b). admitted
group_concat(x order by y) has two arg here. banned now
And some JSON agg will have complicated multi args too. banned now

Got it, make sense.

We can some comments here to make it clearer

planner/core/task.go

fixdb · 2023-01-01T08:42:17Z

planner/core/task.go

+			// even for distinct agg multi args like count(distinct a,b), either null of a,b will cause count func to skip this row.
+			// so we only need to set the either one arg of them to be a case when expression to target for its self-group:
+			// Expr = (case when groupingID = ? then arg else null end)
+			// count(distinct a,b) => count(distinct (case when groupingID = targetID then a else null end), b)


Hmm, I remembered we don't need the case when here, at least for built-in agg functions.

it's a standard way, actually, we can remove this projection to let distinct-agg and normal-agg aggregate on whole scope data. (null rows included but not counted for these null-strict built aggs), what do you think?

Sounds good. count(*) is an exception.

count(*) has already been rewritten as count(1) when entering here

planner/core/testdata/enforce_mpp_suite_out.json

fixdb · 2023-01-01T09:08:10Z

planner/core/testdata/enforce_mpp_suite_out.json

+        ]
+      },
+      {
+        "SQL": "select count(distinct a, b), count(distinct b), count(c), sum(d) from t",


We don't support it at the moment, is it because TiFlash doesn't support?

it needs some changes: Expand op produces non-fixed schema length(currently just append one more LongLong column --- groupingID), I will do it next step. Otherwise, this TiFlash pr will become huge.

Understood.

planner/core/testdata/enforce_mpp_suite_in.json

fixdb · 2023-01-01T09:13:38Z

planner/core/task.go

+
+	// step2: adjust middle agg
+	// proj4Middle output all the base col from lower op + caseWhen proj cols.（reuse partial agg stats）
+	proj4Middle := new(PhysicalProjection).Init(p.ctx, partialHashAgg.stats, partialHashAgg.SelectBlockOffset())


If there is no non-distinct agg function (all of them are distinct functions), do we still need the projection operator?

proj4Partial: project for normal-agg arg with case-when
proj4Middle: project for distinct-agg arg with case-when

as mentioned in the comment above, if do it in a more efficient/non-standard way, we can remove case-when to let both null-strict agg aggregate on whole scope data

AilinKid · 2023-01-04T07:21:56Z

new refactor, simplify the computation since all current TiDB aggregation is null-stricted, so we can just let all distinct aggregation aggregate on whole-scope data (including their expanded rows, which means no case-when expression to select targeted data), because those expanded rows are all filled with null values for your current targeted agg, these rows all naturally ignored in it's agg computation logic.

for normal agg, cases like: select count(distinct a), count(distinct b), count(c) from t; since c is not one of the grouping column, count(c) will get a doubled-result when it aggregates on whole-scope data since column c is not filled with null in other grouping sets. So normal agg still needs case-when expression to targeted for their cared group data.

// select count(distinct a), count(distinct b), count(c) from t
//  final agg   : sum(#1), sum(#2), sum(#3)
//                |         |        +--------------------------------------------+
//                |         +--------------------------------+                    |
//                +-------------+                            |                    |
//                              v                            v                    v
//  middle agg  : count(distinct caseWhen4a) as #1, count(distinct caseWhen4b) as #2, sum(#4) as #3    # all partial result
//                                   |                              |
//                                   +---------------------+        |
//                                                         v        v
//  proj4Middle : a, b, groupingID, #4(partial res), caseWhen4a, caseWhen4b               # caseWhen4a & caseWhen4a depend on groupingID and a, b
//                                      |
//                                      |
//  exchange hash partition: <a, b, groupingID>                                # shuffle data by <a,b,groupingID>
//                                      |
//                                      v
//  partial agg : a, b, groupingID, count(caseWhen4c) as #4           # since group by a, b, groupingID we can directly output all this three.
//                                         |
//                                         v
//  proj4Partial: a, b, c, groupingID, caseWhen4c            # caseWhen4c depend on groupingID and c
//
//  expand: a, b, c, groupingID                        # appended groupingID to diff group set
//
//  lower source: a, b, c                       # don't care what the lower source is

// since TiDB currently doesn't have supported non-null strict aggregation, we can remove the case-when projection
// in the cases above and let distinct-agg directly to aggregate on the whole-scope data. As a result,
// the new refactored plan will be like below.

// select count(distinct a), count(distinct b), count(c) from t
//  final agg   : sum(#1), sum(#2), sum(#3)
//                |         |        +------------------------+
//                |         +-----------------+               |
//                +-----------+               |               |
//                            v               v               v
//  middle agg  : count(a) as #1, count(b) as #2, sum(#4) as #3          # all partial result, since current agg is all null-stricted，distinct agg is targeting whole-scope data                                       
//  
//
//  exchange hash partition: <a, b, groupingID>                     # shuffle data by <a,b,groupingID>
//                                 
//
//  partial agg : a, b, groupingID, count(caseWhen4c) as #4    # since current agg is all null-stricted，normal agg is targeting whole-scope data
//
//  proj4Partial: a, b, c, groupingID, caseWhen4c         # caseWhen4c depend on groupingID and c
//
//  expand: a, b, c, groupingID                      # appended groupingID to diff group set
//
//  lower source: a, b, c                       # don't care what the lower source is

winoros

I haven't taken a careful look at how we merge the grouping sets. Others parts LGTM

winoros · 2023-01-16T18:09:25Z

planner/core/task.go

+			// groupingID now is the offset of target grouping in GroupingSets.
+			// todo: it may be changed after grouping set merge in the future.
+			fun.GroupingID = int64(len(GroupingSets))
+		} else if len(fun.Args) > 1 {


We can some comments here to make it clearer

elsa0520

LGTM

elsa0520 · 2023-01-20T05:45:46Z

expression/grouping_sets.go

+		// for every group set，try to find its position to insert if possible，otherwise create a new grouping set.
+		for j := len(oneNewGroupingSet) - 1; j >= 0; j-- {
+			cur := oneNewGroupingSet[j]
+			if targetOne.SubSetOf(cur) {


Fixed, LGTM

Signed-off-by: AilinKid <[email protected]>

AilinKid · 2023-02-20T09:31:37Z

/merge

ti-chi-bot · 2023-02-20T09:31:41Z

This pull request has been accepted and is ready to merge.

Commit hash: b353681

Signed-off-by: AilinKid <[email protected]>

AilinKid · 2023-02-20T10:00:05Z

/merge

ti-chi-bot · 2023-02-20T10:00:11Z

This pull request has been accepted and is ready to merge.

Commit hash: d9f693b

AilinKid · 2023-02-20T10:15:27Z

/retest

AilinKid · 2023-02-20T10:47:01Z

/retest

ti-chi-bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 15, 2022

AilinKid changed the title ~~multi distinct agg initial commit~~ planner, mock-copr: multi distinct agg initial commit Dec 15, 2022

ti-chi-bot removed the do-not-merge/invalid-title label Dec 15, 2022

AilinKid changed the title ~~planner, mock-copr: multi distinct agg initial commit~~ planner, expression: support multi distinct agg under mpp mode Dec 15, 2022

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 15, 2022

AilinKid changed the title ~~planner, expression: support multi distinct agg under mpp mode~~ planner, expression: support multi-distinct agg under MPP mode Dec 15, 2022

AilinKid requested review from fixdb and winoros December 15, 2022 10:12

winoros reviewed Dec 15, 2022

View reviewed changes

AilinKid mentioned this pull request Dec 26, 2022

plan, executor: implement Expand operator for grouping sets pingcap/tiflash#6545

Merged

12 tasks

winoros reviewed Dec 27, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2022

AilinKid force-pushed the multi-distinct-agg branch from 84677f9 to 8748dad Compare December 27, 2022 15:10

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2022

winoros reviewed Dec 29, 2022

View reviewed changes

expression/constant.go Outdated Show resolved Hide resolved

planner/core/task.go Outdated Show resolved Hide resolved

planner/core/task.go Outdated Show resolved Hide resolved

planner/core/task.go Outdated Show resolved Hide resolved

planner/core/task.go Outdated Show resolved Hide resolved

elsa0520 reviewed Dec 29, 2022

View reviewed changes

fixdb reviewed Jan 1, 2023

View reviewed changes

planner/core/testdata/enforce_mpp_suite_out.json Outdated Show resolved Hide resolved

fixdb reviewed Jan 1, 2023

View reviewed changes

planner/core/testdata/enforce_mpp_suite_in.json Outdated Show resolved Hide resolved

fixdb reviewed Jan 1, 2023

View reviewed changes

winoros reviewed Jan 16, 2023

View reviewed changes

elsa0520 approved these changes Jan 20, 2023

View reviewed changes

AilinKid added 13 commits February 20, 2023 17:09

fix lint

8d38687

Signed-off-by: AilinKid <[email protected]>

fix old test

a4e0506

Signed-off-by: AilinKid <[email protected]>

make fmt

347df77

Signed-off-by: AilinKid <[email protected]>

banned cases need column clone

5e8f4ae

Signed-off-by: AilinKid <[email protected]>

make fmt

83bd74f

remove debug log

854a3fa

Signed-off-by: AilinKid <[email protected]>

address elsa comment

ac6e5a2

Signed-off-by: AilinKid <[email protected]>

address haisheng's comment

6322c55

Signed-off-by: AilinKid <[email protected]>

left distinct count agg to aggregate on whole-scope data

46cafc2

Signed-off-by: AilinKid <[email protected]>

fix test

bd49460

Signed-off-by: AilinKid <[email protected]>

make fmt

98dd328

Signed-off-by: AilinKid <[email protected]>

fix test

7fd6766

Signed-off-by: AilinKid <[email protected]>

add a switch to avoid plan generation fail before tiflash side merged

b353681

Signed-off-by: AilinKid <[email protected]>

AilinKid force-pushed the multi-distinct-agg branch from f70d1e7 to b353681 Compare February 20, 2023 09:27

AilinKid requested a review from a team as a code owner February 20, 2023 09:27

qw4990 approved these changes Feb 20, 2023

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 20, 2023

fix other test

d9f693b

Signed-off-by: AilinKid <[email protected]>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 20, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 20, 2023

AilinKid removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2023

AilinKid merged commit 4ccce9c into pingcap:master Feb 20, 2023

AilinKid mentioned this pull request Feb 23, 2023

planner: add 2-stage multi-distinct agg choice for multi distinct agg under MPP mode #41684

Open

12 tasks

AilinKid mentioned this pull request Mar 28, 2023

Implement Underlying Grouping Sets #42631

Closed

planner, expression: support multi-distinct agg under MPP mode #39973

planner, expression: support multi-distinct agg under MPP mode #39973

Conversation

AilinKid commented Dec 15, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Demo

Check List

Release note

ti-chi-bot commented Dec 15, 2022 • edited Loading

AilinKid commented Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

AilinKid Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Dec 28, 2022

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

AilinKid Dec 30, 2022 • 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

AilinKid Jan 4, 2023 • 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

AilinKid Jan 4, 2023 • 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

AilinKid commented Jan 4, 2023 • edited Loading

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsa0520 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Feb 20, 2023

ti-chi-bot commented Feb 20, 2023

AilinKid commented Feb 20, 2023

ti-chi-bot commented Feb 20, 2023

AilinKid commented Feb 20, 2023

AilinKid commented Feb 20, 2023

AilinKid commented Dec 15, 2022 •

edited

Loading

ti-chi-bot commented Dec 15, 2022 •

edited

Loading

AilinKid commented Dec 18, 2022 •

edited

Loading

AilinKid Dec 27, 2022 •

edited

Loading

AilinKid Dec 30, 2022 •

edited

Loading

AilinKid Jan 4, 2023 •

edited

Loading

AilinKid Jan 4, 2023 •

edited

Loading

AilinKid commented Jan 4, 2023 •

edited

Loading