planner: fix bugs related to TIDB_INLJ hint by foreyes · Pull Request #11253 · pingcap/tidb

foreyes · 2019-07-15T08:35:01Z

What problem does this PR solve?

SQL Hint doesn't work correctly in some cases:

mysql> explain select /*+ TIDB_INLJ(t1) */ t1.b, t2.a from t t1, t t2 where t1.b = t2.a;
+------------------------+----------+------+---------------------------------------------------------------------------+
| id                     | count    | task | operator info                                                             |
+------------------------+----------+------+---------------------------------------------------------------------------+
| IndexJoin_9            | 12500.00 | root | inner join, inner:TableReader_8, outer key:test.t1.b, inner key:test.t2.a |
| ├─TableReader_11       | 10000.00 | root | data:TableScan_10                                                         |
| │ └─TableScan_10       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:false, stats:pseudo               |
| └─TableReader_8        | 1.00     | root | data:TableScan_7                                                          |
|   └─TableScan_7        | 1.00     | cop  | table:t2, range: decided by [test.t1.b], keep order:false, stats:pseudo   |
+------------------------+----------+------+---------------------------------------------------------------------------+
5 rows in set (0.00 sec)

mysql> explain select /*+ TIDB_INLJ(t1) */ t1.a, t2.a, t3.a from t t1, t t2, t t3 where t1.a = t2.a and t2.a = t3.a;
+----------------------------+----------+------+----------------------------------------------------------------------------+
| id                         | count    | task | operator info                                                              |
+----------------------------+----------+------+----------------------------------------------------------------------------+
| IndexJoin_12               | 15625.00 | root | inner join, inner:TableReader_11, outer key:test.t2.a, inner key:test.t3.a |
| ├─IndexJoin_19             | 12500.00 | root | inner join, inner:TableReader_18, outer key:test.t2.a, inner key:test.t1.a |
| │ ├─TableReader_18         | 1.00     | root | data:TableScan_17                                                          |
| │ │ └─TableScan_17         | 1.00     | cop  | table:t1, range: decided by [test.t2.a], keep order:false, stats:pseudo    |
| │ └─TableReader_21         | 10000.00 | root | data:TableScan_20                                                          |
| │   └─TableScan_20         | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo                |
| └─TableReader_11           | 1.00     | root | data:TableScan_10                                                          |
|   └─TableScan_10           | 1.00     | cop  | table:t3, range: decided by [test.t2.a], keep order:false, stats:pseudo    |
+----------------------------+----------+------+----------------------------------------------------------------------------+
8 rows in set (0.00 sec)

What is changed and how it works?

Fix the logic of tryToGetIndexJoin, it won't return incorrect forced any more.
Fix function extractTableAlias, to ensure the table alias is available.

Check List

Tests

Unit test

Side effects

Change physical optimize behaviors

codecov · 2019-07-15T08:39:38Z

Codecov Report

Merging #11253 into master will increase coverage by 0.0069%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11253        +/-   ##
================================================
+ Coverage   81.2684%   81.2753%   +0.0069%     
================================================
  Files           423        423                
  Lines         90094      90106        +12     
================================================
+ Hits          73218      73234        +16     
  Misses        11581      11581                
+ Partials       5295       5291         -4

codecov · 2019-07-15T08:39:38Z

Codecov Report

Merging #11253 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11253   +/-   ##
===========================================
  Coverage   81.5135%   81.5135%           
===========================================
  Files           423        423           
  Lines         91045      91045           
===========================================
  Hits          74214      74214           
  Misses        11541      11541           
  Partials       5290       5290

foreyes · 2019-07-15T08:45:36Z

/run-all-tests

foreyes · 2019-07-15T08:51:48Z

PTAL @winoros

winoros

codes lgtm

foreyes · 2019-07-16T03:41:34Z

/run-all-tests

lzmhhh123 · 2019-07-16T04:12:08Z

+		tblName := p.Schema().Columns[0].TblName.L
+		for _, column := range p.Schema().Columns {
+			if column.TblName.L != tblName {
+				return nil


What about returns all alias table name here. Then index lookup join hint may be effective in more situation.

This feature will be added later, we will have some Join Order Hints. By the way, index lookup join doesn't seem to be able to work when there are multiple alias.

Because there is no available index in this situation.

tidb(localhost:4000) > drop table if exists t; Query OK, 0 rows affected (0.02 sec) tidb(localhost:4000) > create table t(a int, b int, primary key(a)); Query OK, 0 rows affected (0.00 sec) tidb(localhost:4000) > explain select /*+ TIDB_INLJ(t1) */ t1.b, t2.a from t t1, t t2, t t3 where t1.a = t2.a and t1.a = t3.a; +----------------------------+----------+------+------------------------------------------------------------+ | id | count | task | operator info | +----------------------------+----------+------+------------------------------------------------------------+ | Projection_9 | 15625.00 | root | test.t1.b, test.t2.a | | └─MergeJoin_10 | 15625.00 | root | inner join, left key:test.t3.a, right key:test.t1.a | | ├─TableReader_17 | 10000.00 | root | data:TableScan_16 | | │ └─TableScan_16 | 10000.00 | cop | table:t3, range:[-inf,+inf], keep order:true, stats:pseudo | | └─MergeJoin_18 | 12500.00 | root | inner join, left key:test.t1.a, right key:test.t2.a | | ├─TableReader_25 | 10000.00 | root | data:TableScan_24 | | │ └─TableScan_24 | 10000.00 | cop | table:t1, range:[-inf,+inf], keep order:true, stats:pseudo | | └─TableReader_27 | 10000.00 | root | data:TableScan_26 | | └─TableScan_26 | 10000.00 | cop | table:t2, range:[-inf,+inf], keep order:true, stats:pseudo | +----------------------------+----------+------+------------------------------------------------------------+ 9 rows in set, 1 warning (0.00 sec)

Actually, in this case, t2.a and t3.a are indices. But we can't choose the index join with TIDB_INLJ hint.

Because Hint only influence joins involving t1 , it should not enforce parent join to use Index Join. The parent join compares Index Join + Index Join(Hint works) and Merge Join + Merge Join(Hint doesn't work because Merge Join require order, t1 can't be inner table in this situation), and choose the second as the best one.

If change the condition into t1.a = t2.a AND t2.a = t3.a, optimizer will choose Merge Join + Index Join, because t1 is able to be inner table in this situation. Actually, they are the same...Maybe we can do some optimize?

Kenan & me are going to enhance Optimizer Hints.

Use Hint TIDB_INLJ(t1, t3) may lead to Index Join + Index Join.

foreyes · 2019-07-16T04:27:06Z

/run-all-tests

alivxxx · 2019-07-17T06:54:04Z


+// This function will return the table alias of a given logical plan.
+// If there're multiple tables, it will return nil.
+// This function will be further developed in the future.


This line provides little info and makes me curious, you can add more details like what will be developed and why.

I have rewrite the comments... I find out it may only be influenced by Schema refactoring.

alivxxx

LGTM

foreyes · 2019-07-18T02:28:54Z

/run-all-tests

foreyes · 2019-07-19T06:41:56Z

/run-all-tests

foreyes · 2019-07-19T06:55:54Z

/run-all-tests

lzmhhh123 · 2019-07-19T12:24:24Z

/run-all-tests

foreyes · 2019-07-22T03:21:32Z

/run-all-tests

foreyes · 2019-07-22T03:42:13Z

/run-all-tests

foreyes · 2019-07-22T04:57:41Z

After my strange operation, the approve is lost. @zz-jason

foreyes · 2019-07-22T05:01:40Z

I find out it's not the latest version, I have to reset to another commit. @zz-jason

foreyes · 2019-07-22T05:18:12Z

Dismiss again. I used force-push too carelessly, I'll pay attention next time. @zz-jason

sre-bot · 2019-07-22T05:28:34Z

cherry pick to release-2.1 in PR #11361

sre-bot · 2019-07-22T05:30:07Z

cherry pick to release-3.0 in PR #11362

foreyes added 3 commits July 15, 2019 13:00

fix logic of extractTableName and tryToGetIndexJoin

f268cf1

add unit test

c23b6b1

fix test case

b3e6506

foreyes requested review from eurekaka and winoros July 15, 2019 08:36

foreyes changed the title ~~planner: fix problems related to SQL Hint~~ planner: fix problems related to optimizer_hint Jul 15, 2019

foreyes requested a review from lzmhhh123 July 15, 2019 08:43

foreyes added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Jul 15, 2019

foreyes added the status/all tests passed label Jul 15, 2019

winoros reviewed Jul 15, 2019

View reviewed changes

Comment thread planner/core/logical_plan_builder.go

winoros reviewed Jul 16, 2019

View reviewed changes

Comment thread planner/core/logical_plan_builder.go

foreyes added 2 commits July 16, 2019 11:33

add comment

277a20e

Merge branch 'master' into foreyes_dev

943444a

lzmhhh123 changed the title ~~planner: fix problems related to optimizer_hint~~ planner: fix bugs related to TIDB_INLJ hint Jul 16, 2019

lzmhhh123 reviewed Jul 16, 2019

View reviewed changes

foreyes requested a review from alivxxx July 16, 2019 04:34

alivxxx reviewed Jul 17, 2019

View reviewed changes

foreyes force-pushed the foreyes_dev branch 2 times, most recently from a8f2fa8 to 60f40cc Compare July 17, 2019 07:41

rewrite comments

41c1828

foreyes force-pushed the foreyes_dev branch from 60f40cc to 41c1828 Compare July 17, 2019 07:45

alivxxx reviewed Jul 17, 2019

View reviewed changes

zz-jason added needs-cherry-pick-2.1 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 22, 2019

foreyes closed this Jul 22, 2019

foreyes force-pushed the foreyes_dev branch from edb2869 to 4b928a5 Compare July 22, 2019 03:32

foreyes reopened this Jul 22, 2019

foreyes dismissed zz-jason’s stale review via 4e73246 July 22, 2019 03:33

zz-jason previously approved these changes Jul 22, 2019

View reviewed changes

rewrite comments & clear code

74216c7

foreyes dismissed zz-jason’s stale review via 74216c7 July 22, 2019 05:13

foreyes force-pushed the foreyes_dev branch from d453cf3 to 74216c7 Compare July 22, 2019 05:13

Merge branch 'master' into foreyes_dev

a9b60c9

zz-jason approved these changes Jul 22, 2019

View reviewed changes

foreyes merged commit 5f7f43a into pingcap:master Jul 22, 2019

foreyes deleted the foreyes_dev branch July 22, 2019 05:28

sre-bot mentioned this pull request Jul 22, 2019

planner: fix bugs related to TIDB_INLJ hint (#11253) #11361

Merged

sre-bot mentioned this pull request Jul 22, 2019

planner: fix bugs related to TIDB_INLJ hint (#11253) #11362

Merged

eurekaka pushed a commit that referenced this pull request Jul 23, 2019

planner: fix bugs related to TIDB_INLJ hint (#11253) (#11362)

db533f5

zz-jason pushed a commit that referenced this pull request Jul 23, 2019

planner: fix bugs related to TIDB_INLJ hint (#11253) (#11361)

995b363

Conversation

foreyes commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

codecov Bot commented Jul 15, 2019

Codecov Report

Uh oh!

codecov Bot commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

foreyes commented Jul 15, 2019

Uh oh!

foreyes commented Jul 15, 2019

Uh oh!

Uh oh!

winoros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

foreyes commented Jul 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foreyes commented Jul 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alivxxx left a comment

Choose a reason for hiding this comment

Uh oh!

foreyes commented Jul 18, 2019

Uh oh!

foreyes commented Jul 19, 2019

Uh oh!

foreyes commented Jul 19, 2019

Uh oh!

lzmhhh123 commented Jul 19, 2019

Uh oh!

foreyes commented Jul 22, 2019

Uh oh!

foreyes commented Jul 22, 2019

Uh oh!

foreyes commented Jul 22, 2019

Uh oh!

foreyes commented Jul 22, 2019

Uh oh!

foreyes commented Jul 22, 2019

Uh oh!

sre-bot commented Jul 22, 2019

Uh oh!

sre-bot commented Jul 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

foreyes commented Jul 15, 2019 •

edited

Loading

codecov Bot commented Jul 15, 2019 •

edited

Loading