plan: convert in subquery to agg and inner join #7531

winoros · 2018-08-29T08:09:59Z

What problem does this PR solve?

Implement #7205.

What is changed and how it works?

I decided to unfold the subquery first. And implement this one in future new planner. But in testing, unfolding subquery's performance the far more worse than rewriting.(Listed below).
So decides to implement it in current planner.

This rewriting will cause new aggregation or projection. So add a aggregate elimination rule and one more project elimination.

Note that we can only do one project elimination, just at the place where we add the new project elimination. Only do once will change the column name in some explain result, though won't change the plan structure. I've not decided which one is better.

Check List

Tests

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

Related changes

Need to update the documentation. Yes, the doc about session variable should be updated
Need to be included in the release note. Yes, Rewrite in subquery to inner join with aggregation.

winoros · 2018-08-29T08:11:54Z

mysql> set @@session.tidb_opt_insubquery_unfold = 1;
Query OK, 0 rows affected (0.00 sec)
mysql> select count(*) from lineitem where l_orderkey in (select l_orderkey from lineitem limit 800000);
+----------+
| count(*) |
+----------+
|   800017 |
+----------+
1 row in set (18.68 sec)
mysql> set @@session.tidb_opt_insubquery_unfold = 0;
Query OK, 0 rows affected (0.00 sec)
mysql> select count(*) from lineitem where l_orderkey in (select l_orderkey from lineitem limit 800000);
+----------+
| count(*) |
+----------+
|   800020 |
+----------+
1 row in set (5.92 sec)
mysql> select count(*) from lineitem where l_orderkey in (select l_orderkey from lineitem limit 800000);
+----------+
| count(*) |
+----------+
|   800020 |
+----------+
1 row in set (37.81 sec)

First one is unfolding, second is rewriting, third is do nothing.

Another test in TPCH Query 18. Though due to the join order and not so good row count estimation. This rule currently cannot take effect on it.
But if we rewrite the join order, it can get results at 120s~. Before this pr, it's 240s~.

winoros · 2018-08-29T08:17:04Z

plan/rule_aggregation_elimination.go

+	return nil
+}
+
+func (a *aggregationEliminater) convertAggToProj(agg *LogicalAggregation) *LogicalProjection {


Most methods are copied from aggregate push down. I think it's not good to leave them as function that anyone can call.
Extract a struct that can be used both by aggregate elimination and aggregate push down, make them as method of this struct ?

Hmm, can we set aggregationOptimizer as an anonymous field of aggregationEliminater? then we only need to override the optimize method for aggregationEliminater.

aggregationOptimizer also handles the aggregate elimination, why not use it here? because AllowAggPushDown is false by default?

Extract a struct that can be used both by aggregate elimination and aggregate push down, make them as method of this struct ?

IMHO, aggregationOptimizer should be this struct, and we should add an aggregationPushDownSolver as subclass of it to hold the aggregate pushdown logic.

aggregationOptimizer also handles the aggregate elimination, why not use it here? because AllowAggPushDown is false by default?

Yes, that one is mainly to push down aggregation. The elimination is done there for convenience and due to some limitation the current planner has.

IMHO, aggregationOptimizer should be this struct, and we should add an aggregationPushDownSolver as subclass of it to hold the aggregate pushdown logic.

But there's no overriding in golang. So we cannot make one containing another one. They both have a recursive method optimize to implement logicalOptRule interface.

zz-jason · 2018-08-30T09:04:54Z

@eurekaka PTAL

eurekaka · 2018-08-31T17:27:28Z

plan/optimizer.go

 	flagDecorrelate
+	flagEliminateProjection2
 	flagMaxMinEliminate


Any reason why flagEliminateProjection2 is applied after flagDecorrelate?

Seems i can just place it after the ElimiateAgg before the Decorrelate. This will be done in the newly coming commit.

Ok tpch Q20's result will change if we change the position.
This should be caused that we don't maintain the KeyInfo very carefully.
So move it after the decorrelation.

eurekaka · 2018-08-31T17:50:29Z

plan/rule_aggregation_elimination.go

+	return nil
+}
+
+func (a *aggregationEliminater) convertAggToProj(agg *LogicalAggregation) *LogicalProjection {


Hmm, can we set aggregationOptimizer as an anonymous field of aggregationEliminater? then we only need to override the optimize method for aggregationEliminater.

aggregationOptimizer also handles the aggregate elimination, why not use it here? because AllowAggPushDown is false by default?

Extract a struct that can be used both by aggregate elimination and aggregate push down, make them as method of this struct ?

IMHO, aggregationOptimizer should be this struct, and we should add an aggregationPushDownSolver as subclass of it to hold the aggregate pushdown logic.

eurekaka · 2018-09-01T06:17:44Z

plan/expression_rewriter.go

-		return v, true
+	// If it's not the form of `not in (SUBQUERY)`, has no correlated column and don't need to append a scalar value. We can rewrite it to inner join.
+	if er.ctx.GetSessionVars().AllowInSubqueryRewriting && !v.Not && !asScalar && len(np.extractCorrelatedCols()) == 0 {
+		// We need to try to eliminate the agg and the projection produced by this operation.


I am confused about the expressionRewriter::asScalar field, could you please shed some light on the purpose of this field? thx

You can take a look at buildSemiJoin. The asScalar indicates that whether this semi join need to be considered as scalar and output a special row to the plan above it.

zz-jason · 2018-08-30T09:05:54Z

cmd/explaintest/r/explain_easy.result

@@ -5,7 +5,6 @@ create table t2 (c1 int unique, c2 int);
 insert into t2 values(1, 0), (2, 1);
 create table t3 (a bigint, b bigint, c bigint, d bigint);
 create table t4 (a int, b int, c int, index idx(a, b), primary key(a));
-set @@session.tidb_opt_insubquery_unfold = 1;


It's better to keep this test and add another test which disables tidb_opt_insubquery_unfold.

but this variable is removed

Yeah, I just noticed that after reviewing all the code changes.

Just changed the name. We still test both open it and close it.

zz-jason · 2018-09-03T03:38:28Z

sessionctx/variable/tidb_vars.go

-	// tidb_opt_insubquery_unfold is used to enable/disable the optimizer rule of in subquery unfold.
-	TiDBOptInSubqUnFolding = "tidb_opt_insubquery_unfold"
+	// tidb_opt_insubquery_rewriting is used to enable/disable the optimizer rule of rewriting IN subquery.
+	TiDBOptInSubqRewriting = "tidb_opt_insubquery_rewriting"


how about:

s/tidb_opt_insubquery_rewriting/tidb_opt_insubquery_to_innerjoin/ s/TiDBOptInSubqRewriting/TiDBOptInSubqueryToInnerJoin/

The actual form is inner join together with an aggregation. So ToInnerJoin is not complete but InnerJoinAndAgg it too long.

I think "tidb_opt_insubquery_to_innerjoin" is clearer than "tidb_opt_insubquery_rewriting", because the key information about this transformation is displayed in the variable name.

maybe "tidb_opt_insubquery_to_innerjoin" can be shorten to "tidb_insubq_to_innerjoin"

zz-jason · 2018-09-03T03:38:58Z

sessionctx/variable/session.go

-	// AllowInSubqueryUnFolding can be set to true to fold in subquery
-	AllowInSubqueryUnFolding bool
+	// AllowInSubqueryRewriting can be set to false to forbid rewriting the semi join to inner join with agg.
+	AllowInSubqueryRewriting bool


how about s/AllowInSubqueryRewriting/InSubqueryToInnerJoin/

zz-jason · 2018-09-17T07:36:07Z

plan/rule_aggregation_elimination.go

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


#7676 already did the same thing, this patch can be removed.

It should be done when merge that into this. Or the test result will be strange.

Ok, let's get #7676 merged as soon as possible.

So we should merge 7680 first...

winoros · 2018-10-08T12:19:03Z

In explain test, there's one projection left though it should be okay to eliminate it. I'm looking into it.
This should not block the review of the main code change in this pr.
Okay, the binary that test used is wrong. It's actually correct.
@zz-jason @eurekaka @lamxTyler PTAL

winoros · 2018-10-09T03:46:32Z

/run-common-test

winoros · 2018-10-09T06:27:51Z

/run-all-tests

planner/core/optimizer.go

zz-jason · 2018-10-18T10:32:19Z

@winoros please merge master && resolve conflicts.

winoros · 2018-10-22T11:41:56Z

@zz-jason updated

winoros · 2018-10-25T11:47:21Z

/run-all-tests

zz-jason · 2018-10-25T11:50:47Z

LGTM

zz-jason · 2018-10-25T11:51:01Z

@eurekaka @lamxTyler PTAL

alivxxx

LGTM

alivxxx · 2018-10-26T03:19:23Z

@winoros Please resolve the conflict.

winoros · 2018-10-30T11:03:26Z

./run-all-tests

winoros added 10 commits August 28, 2018 15:50

plan: convert in subquery to agg and inner join

e82e83e

Modify rule. So it won't be worse than before.

4cac41f

rename the variable

6c06056

fix the unit tests.

7b22605

add new rule to eliminate agg by key.

500b54a

fix explain test.

62705f6

variable is not initialized in session.

9f1a980

modify tpch comment.

72ff6ad

add comment.

31372da

fix comment

48018ff

winoros added type/enhancement The issue or PR belongs to an enhancement. sig/planner SIG: Planner release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 29, 2018

winoros commented Aug 29, 2018

View reviewed changes

winoros added 2 commits August 29, 2018 16:18

fix gofmt.

8c8ed42

Merge branch 'master' into insubq-to-agg-and-join

9073856

eurekaka reviewed Aug 31, 2018

View reviewed changes

eurekaka reviewed Sep 1, 2018

View reviewed changes

zz-jason reviewed Sep 3, 2018

View reviewed changes

winoros added 4 commits September 12, 2018 15:35

Merge branch 'master' into insubq-to-agg-and-join

0c5dd50

extract same part into struct

81ba7ec

rename variable

d21347b

fix test

1d5523c

winoros mentioned this pull request Sep 12, 2018

planner/core: separate aggPrune from aggPushDown #7676

Merged

zz-jason reviewed Sep 17, 2018

View reviewed changes

winoros added 2 commits October 8, 2018 20:13

Merge branch 'master' into insubq-to-agg-and-join

f40d376

delete unused file

f26b701

Merge branch 'master' into insubq-to-agg-and-join

be51b0e

zz-jason reviewed Oct 11, 2018

View reviewed changes

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

winoros added 2 commits October 22, 2018 19:14

Merge branch 'master' into insubq-to-agg-and-join

5ef7f65

fix panic in test.

091b5b7

winoros added 3 commits October 25, 2018 19:33

address comment

dd93228

Merge branch 'master' into insubq-to-agg-and-join

e08c2f6

fix explain_test

d933826

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 25, 2018

alivxxx reviewed Oct 26, 2018

View reviewed changes

winoros added 2 commits October 30, 2018 17:46

Merge branch 'master' into insubq-to-agg-and-join

8bc9c80

add it back

e788281

alivxxx previously approved these changes Oct 30, 2018

View reviewed changes

fix explain test

fe1370c

winoros dismissed alivxxx’s stale review via fe1370c October 30, 2018 10:59

alivxxx approved these changes Oct 30, 2018

View reviewed changes

alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 30, 2018

winoros merged commit 696ef7b into pingcap:master Oct 30, 2018

winoros deleted the insubq-to-agg-and-join branch October 30, 2018 11:19

winoros mentioned this pull request Jan 8, 2019

transform semi join to inner join #7205

Closed

plan: convert in subquery to agg and inner join #7531

plan: convert in subquery to agg and inner join #7531

Conversation

winoros commented Aug 29, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

winoros commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

zz-jason commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros Sep 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros commented Oct 8, 2018 • edited Loading

winoros commented Oct 9, 2018

winoros commented Oct 9, 2018

zz-jason commented Oct 18, 2018

winoros commented Oct 22, 2018

winoros commented Oct 25, 2018

zz-jason commented Oct 25, 2018

zz-jason commented Oct 25, 2018

alivxxx left a comment

Choose a reason for hiding this comment

alivxxx commented Oct 26, 2018

winoros commented Oct 30, 2018

winoros commented Aug 29, 2018 •

edited

Loading

winoros Sep 5, 2018 •

edited

Loading

winoros Sep 12, 2018 •

edited

Loading

winoros commented Oct 8, 2018 •

edited

Loading