planner: add warning when the table name of indexHint cannot be found #15517

francis0407 · 2020-03-20T08:17:39Z

What problem does this PR solve?

Problem Summary:

Throw a warning when the index hint is not applicable(i.e. cannot find the table).

What is changed and how it works?

What's Changed:
Before this PR:

mysql> explain select /*+ use_index(p, a) */ * from t;
+-----------------------+----------+-----------+-----------------------------------------+
| id                    | estRows  | task      | operator info                           |
+-----------------------+----------+-----------+-----------------------------------------+
| TableReader_5         | 10000.00 | root      | data:TableFullScan_4                    |
| └─TableFullScan_4     | 10000.00 | cop[tikv] | table:t, keep order:false, stats:pseudo |
+-----------------------+----------+-----------+-----------------------------------------+
2 rows in set (0.00 sec)

After this PR:

mysql> explain select /*+ use_index(p, a) */ * from t;
+-----------------------+----------+-----------+-----------------------------------------+
| id                    | estRows  | task      | operator info                           |
+-----------------------+----------+-----------+-----------------------------------------+
| TableReader_5         | 10000.00 | root      | data:TableFullScan_4                    |
| └─TableFullScan_4     | 10000.00 | cop[tikv] | table:t, keep order:false, stats:pseudo |
+-----------------------+----------+-----------+-----------------------------------------+
2 rows in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+------------------------------------------+
| Level   | Code | Message                                  |
+---------+------+------------------------------------------+
| Warning | 1815 | IndexHint for table p is not applicable. |
+---------+------+------------------------------------------+
1 row in set (0.00 sec)

How it Works:

Check if there are unused tableHints after building the select statement.

Check List

Tests

Unit test

francis0407 · 2020-03-20T08:19:49Z

planner/core/logical_plan_builder.go

+func (b *PlanBuilder) appendUnmatchedIndexHintWarning(indexHints []indexHintInfo) {
+	for _, hint := range indexHints {
+		if !hint.matched {
+			errMsg := fmt.Sprintf("IndexHint for table %s is not applicable.", hint.tblName)


Any suggestion for the warning message? cc @eurekaka @zz-jason

how about:

fmt.Sprintf( "%s(%d.%s, %s) is inapplicable, check whether the table(%s.%s) or index(%s) exists", hint.indexHint.HintType, hint.dbName, hint.tblName, ... )

Why not directly call the ast.HintTable.restore?

@lzmhhh123 indexHint is ast.IndexHint, it doesn't contain DB or Table when use restore.

codecov · 2020-03-20T08:24:13Z

Codecov Report

Merging #15517 into master will decrease coverage by 0.3667%.
The diff coverage is 92.5000%.

@@               Coverage Diff                @@
##             master     #15517        +/-   ##
================================================
- Coverage   80.7970%   80.4303%   -0.3668%     
================================================
  Files           503        504         +1     
  Lines        136021     134550      -1471     
================================================
- Hits         109901     108219      -1682     
- Misses        17687      17842       +155     
- Partials       8433       8489        +56

lzmhhh123 · 2020-03-20T09:08:03Z

planner/core/logical_plan_builder.go

+func (b *PlanBuilder) appendUnmatchedIndexHintWarning(indexHints []indexHintInfo) {
+	for _, hint := range indexHints {
+		if !hint.matched {
+			errMsg := fmt.Sprintf("IndexHint for table %s is not applicable.", hint.tblName)


Why not directly call the ast.HintTable.restore?

planner/core/planbuilder.go

…int_warning # Conflicts: # planner/core/integration_test.go

planner/core/logical_plan_builder.go

zz-jason · 2020-03-23T09:42:14Z

planner/core/logical_plan_builder.go

+			} else {
+				hintTypeString = hint.hintTypeString()
+			}
+			errMsg := fmt.Sprintf("%s(%s) is inapplicable, check whether the table(%s.%s) exists",


After thinking again, how about only produce one warning for all the inapplicable index hints? It would help to reduce the warning count when a lot of hints are inapplicable.

IMHO, that's not really necessary. One warning for one hint is more helpful to identify the syntax error.

lzmhhh123

LGTM

zz-jason

LGTM

XuHuaiyu · 2020-03-27T02:32:08Z

@francis0407 Please resolve the conflicts

…int_warning # Conflicts: # planner/core/testdata/plan_suite_in.json # planner/core/testdata/plan_suite_out.json

…int_warning

francis0407 · 2020-03-27T02:51:16Z

Conflicts are resovled. PTAL @XuHuaiyu

XuHuaiyu · 2020-03-27T03:28:00Z

/merge

sre-bot · 2020-03-27T03:28:13Z

/run-all-tests

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot · 2020-03-27T03:33:02Z

cherry pick to release-3.1 in PR #15762

…pingcap#15517)

sre-bot · 2020-03-27T03:33:56Z

cherry pick to release-4.0 in PR #15763

add warning for unapplicable index hint

24e1585

francis0407 requested a review from a team as a code owner March 20, 2020 08:17

ghost requested review from eurekaka and lzmhhh123 and removed request for a team March 20, 2020 08:17

francis0407 commented Mar 20, 2020

View reviewed changes

lzmhhh123 added sig/planner SIG: Planner needs-cherry-pick-3.1 type/bugfix This PR fixes a bug. labels Mar 20, 2020

lzmhhh123 reviewed Mar 20, 2020

View reviewed changes

francis0407 added 2 commits March 23, 2020 16:00

address comment

79479c4

Merge branch 'master' of https://github.com/pingcap/tidb into index_h…

09637da

…int_warning # Conflicts: # planner/core/integration_test.go

francis0407 requested review from zz-jason and lzmhhh123 March 23, 2020 09:19

zz-jason reviewed Mar 23, 2020

View reviewed changes

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

zz-jason reviewed Mar 23, 2020

View reviewed changes

francis0407 added 2 commits March 23, 2020 17:58

fix use_index_merge hint

62e0b87

Merge branch 'master' into index_hint_warning

d99bbf7

francis0407 added the needs-cherry-pick-4.0 label Mar 24, 2020

lzmhhh123 reviewed Mar 24, 2020

View reviewed changes

lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 24, 2020

zz-jason reviewed Mar 26, 2020

View reviewed changes

zz-jason added status/require-change status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 26, 2020

francis0407 added 3 commits March 27, 2020 10:38

Merge branch 'master' of https://github.com/pingcap/tidb into index_h…

41d7101

…int_warning # Conflicts: # planner/core/testdata/plan_suite_in.json # planner/core/testdata/plan_suite_out.json

resolve conflicts

b1a9d95

Merge remote-tracking branch 'origin/index_hint_warning' into index_h…

63fa8f5

…int_warning

XuHuaiyu approved these changes Mar 27, 2020

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 27, 2020

sre-bot merged commit dd14172 into pingcap:master Mar 27, 2020

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 27, 2020

cherry pick pingcap#15517 to release-3.1

cd02dbc

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot mentioned this pull request Mar 27, 2020

planner: add warning when the table name of indexHint cannot be found (#15517) #15762

Merged

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 27, 2020

planner: add warning when the table name of indexHint cannot be found (…

2cf7a2c

…pingcap#15517)

sre-bot mentioned this pull request Mar 27, 2020

planner: add warning when the table name of indexHint cannot be found (#15517) #15763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: add warning when the table name of indexHint cannot be found #15517

planner: add warning when the table name of indexHint cannot be found #15517

francis0407 commented Mar 20, 2020 •

edited

Loading

francis0407 Mar 20, 2020

zz-jason Mar 20, 2020

lzmhhh123 Mar 20, 2020

francis0407 Mar 23, 2020

codecov bot commented Mar 20, 2020 •

edited

Loading

lzmhhh123 Mar 20, 2020

zz-jason Mar 23, 2020

francis0407 Mar 24, 2020

lzmhhh123 left a comment

zz-jason left a comment

XuHuaiyu commented Mar 27, 2020

francis0407 commented Mar 27, 2020

XuHuaiyu commented Mar 27, 2020

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

planner: add warning when the table name of indexHint cannot be found #15517

planner: add warning when the table name of indexHint cannot be found #15517

Conversation

francis0407 commented Mar 20, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

francis0407 Mar 20, 2020

Choose a reason for hiding this comment

zz-jason Mar 20, 2020

Choose a reason for hiding this comment

lzmhhh123 Mar 20, 2020

Choose a reason for hiding this comment

francis0407 Mar 23, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2020 • edited Loading

Codecov Report

lzmhhh123 Mar 20, 2020

Choose a reason for hiding this comment

zz-jason Mar 23, 2020

Choose a reason for hiding this comment

francis0407 Mar 24, 2020

Choose a reason for hiding this comment

lzmhhh123 left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Mar 27, 2020

francis0407 commented Mar 27, 2020

XuHuaiyu commented Mar 27, 2020

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

francis0407 commented Mar 20, 2020 •

edited

Loading

codecov bot commented Mar 20, 2020 •

edited

Loading