Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: fix bugs and make it more effective in outer join elimination rule. #11160

Merged

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Jul 10, 2019

What problem does this PR solve?

Now, the rule outer join elimination keeps a bug as the following cases:

tidb(localhost:4000) > drop table if exists t1, t2;
Query OK, 0 rows affected (0.00 sec)

tidb(localhost:4000) > create table t1(a int, b int, primary key(a));
Query OK, 0 rows affected (0.01 sec)

tidb(localhost:4000) > create table t2(a int, b int, c int, index ac(a, c));
Query OK, 0 rows affected (0.00 sec)

tidb(localhost:4000) > desc select t2.a sa, t2.c sc from t2 left join t1 on t2.a = t1.a;
+--------------------------+----------+------+------------------------------------------------------------------------+
| id                       | count    | task | operator info                                                          |
+--------------------------+----------+------+------------------------------------------------------------------------+
| Projection_5             | 12500.00 | root | test.t2.a, test.t2.c                                                   |
| └─MergeJoin_6            | 12500.00 | root | left outer join, left key:test.t2.a, right key:test.t1.a               |
|   ├─IndexReader_12       | 10000.00 | root | index:IndexScan_11                                                     |
|   │ └─IndexScan_11       | 10000.00 | cop  | table:t2, index:a, c, range:[NULL,+inf], keep order:true, stats:pseudo |
|   └─TableReader_14       | 10000.00 | root | data:TableScan_13                                                      |
|     └─TableScan_13       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:true, stats:pseudo             |
+--------------------------+----------+------+------------------------------------------------------------------------+
6 rows in set (0.00 sec)

It can't regard the alias column as the same as origin column. After this PR, this case of outer join can be normally eliminated.

tidb(localhost:4000) > desc select t2.a sa, t2.c sc from t2 left join t1 on t2.a = t1.a;
+-------------------+----------+------+-------------------------------------------------------------+                                           │
| id                | count    | task | operator info                                               |                                           │
+-------------------+----------+------+-------------------------------------------------------------+                                           │ 
| TableReader_7     | 10000.00 | root | data:TableScan_6                                            |                                           │
| └─TableScan_6     | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo |                                           │
+-------------------+----------+------+-------------------------------------------------------------+

Other enhancements and bugs fix are referred to issue: #9536, #11167

What is changed and how it works?

Use col.OrigColName instead of col.ColName. Record columns in schema between LogicalAggregation and LogicalJoin. Substitue columns for LogicalProjection and LogicalSelection.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

@winoros
Copy link
Member

winoros commented Jul 10, 2019

You can change

	// check the duplicate agnostic aggregate functions
	if ok, newCols := o.isDuplicateAgnosticAgg(p); ok {
		aggCols = newCols
	}

	newChildren := make([]LogicalPlan, 0, len(p.Children()))

to

	// check the duplicate agnostic aggregate functions
	if ok, newCols := o.isDuplicateAgnosticAgg(p); ok {
		aggCols = newCols
	}
	if proj, ok := p.(*LogicalProjection); ok {
		call expression.ColumnSubstitute to remap the aggCols.
	}
	newChildren := make([]LogicalPlan, 0, len(p.Children()))

Like what projection did when meet column prune and predicate push down.

Also, don't use projection's schema directly. Use its expression to substitute the columns in schema instead.

@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #11160 into master will decrease coverage by 0.2083%.
The diff coverage is 86.6666%.

@@               Coverage Diff               @@
##             master    #11160        +/-   ##
===============================================
- Coverage   81.4843%   81.276%   -0.2084%     
===============================================
  Files           423       423                
  Lines         91285     90125      -1160     
===============================================
- Hits          74383     73250      -1133     
+ Misses        11603     11576        -27     
  Partials       5299      5299

@lzmhhh123 lzmhhh123 force-pushed the dev/fix_bug_in_outer_join_elimination branch from e578d29 to c076c0f Compare July 10, 2019 10:50
@lzmhhh123 lzmhhh123 added the type/enhancement The issue or PR belongs to an enhancement. label Jul 11, 2019
@lzmhhh123 lzmhhh123 changed the title planner: fix column compare bug in outer join elimination rule. planner: fix bugs and make it more effective in outer join elimination rule. Jul 11, 2019
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff patch can also pass the current tests in planner.

diff.txt

planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
@lzmhhh123 lzmhhh123 force-pushed the dev/fix_bug_in_outer_join_elimination branch from 659b996 to b79feca Compare July 12, 2019 05:43
@lzmhhh123 lzmhhh123 force-pushed the dev/fix_bug_in_outer_join_elimination branch from b79feca to f336f2d Compare July 12, 2019 06:50
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination_test.go Outdated Show resolved Hide resolved
@lzmhhh123
Copy link
Contributor Author

PTAL. @winoros @lamxTyler

@lzmhhh123
Copy link
Contributor Author

/run-all-tests

planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
planner/core/rule_join_elimination.go Outdated Show resolved Hide resolved
@winoros winoros removed the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jul 19, 2019
go.sum Outdated
@@ -12,6 +12,7 @@ github.com/blacktear23/go-proxyprotocol v0.0.0-20180807104634-af7a81e8dd0d/go.mo
github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI=
github.com/chzyer/readline v0.0.0-20171208011716-f6d7a1f6fbf3/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI=
github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=
github.com/client9/misspell v0.3.4 h1:ta993UF76GwbvJcIo3Y68y/M3WxlpEHPWIGDkJYwzJI=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not expected change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lzmhhh123 you can use make tidy to clean up the go.sum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the result with make tidy. ╮(╯_╰)╭

@winkyao winkyao added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jul 22, 2019
@lzmhhh123
Copy link
Contributor Author

PTAL. @zz-jason

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 23, 2019
@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@lzmhhh123 lzmhhh123 merged commit 3a1ba35 into pingcap:master Jul 23, 2019
@lzmhhh123 lzmhhh123 deleted the dev/fix_bug_in_outer_join_elimination branch July 23, 2019 05:13
@sre-bot
Copy link
Contributor

sre-bot commented Jul 23, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Jul 23, 2019

cherry pick to release-3.0 failed

lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Jul 26, 2019
eurekaka added a commit to lzmhhh123/tidb that referenced this pull request Jul 28, 2019
zz-jason added a commit to lzmhhh123/tidb that referenced this pull request Jul 29, 2019
sre-bot added a commit to lzmhhh123/tidb that referenced this pull request Jul 29, 2019
sre-bot pushed a commit that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants