From b0b537bea29856d99dc4518cb701d8a4fe8b753c Mon Sep 17 00:00:00 2001 From: Mingcong Han Date: Mon, 23 Mar 2020 17:31:14 +0800 Subject: [PATCH 1/2] throw warnings when no table names in join hints --- planner/core/integration_test.go | 33 ++++++++++ planner/core/logical_plan_builder.go | 52 ++++++++++++--- .../core/testdata/integration_suite_in.json | 15 +++++ .../core/testdata/integration_suite_out.json | 65 +++++++++++++++++++ 4 files changed, 157 insertions(+), 8 deletions(-) diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 2bfe3e798e596..6bdbbdc50353d 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -22,6 +22,7 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/planner/core" + "github.com/pingcap/tidb/sessionctx/stmtctx" "github.com/pingcap/tidb/util/testkit" "github.com/pingcap/tidb/util/testutil" ) @@ -638,3 +639,35 @@ func (s *testIntegrationSuite) TestIssue15546(c *C) { tk.MustExec("create definer='root'@'localhost' view vt(a, b) as select a, b from t") tk.MustQuery("select * from pt, vt where pt.a = vt.a").Check(testkit.Rows("1 1 1 1")) } + +func (s *testIntegrationSuite) TestHintWithoutTableWarning(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t1, t2") + tk.MustExec("create table t1(a int, b int, c int, key a(a))") + tk.MustExec("create table t2(a int, b int, c int, key a(a))") + var input []string + var output []struct { + SQL string + Warnings []string + } + s.testData.GetTestCases(c, &input, &output) + for i, tt := range input { + s.testData.OnRecord(func() { + output[i].SQL = tt + tk.MustQuery(tt) + warns := tk.Se.GetSessionVars().StmtCtx.GetWarnings() + output[i].Warnings = make([]string, len(warns)) + for j := range warns { + output[i].Warnings[j] = warns[j].Err.Error() + } + }) + tk.MustQuery(tt) + warns := tk.Se.GetSessionVars().StmtCtx.GetWarnings() + c.Assert(len(warns), Equals, len(output[i].Warnings)) + for j := range warns { + c.Assert(warns[j].Level, Equals, stmtctx.WarnLevelWarning) + c.Assert(warns[j].Err.Error(), Equals, output[i].Warnings[j]) + } + } +} diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 5f56c5117d9de..06bfb9923d85f 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -2184,6 +2184,16 @@ func (b *PlanBuilder) unfoldWildStar(p LogicalPlan, selectFields []*ast.SelectFi return resultList, nil } +func (b *PlanBuilder) pushHintWithoutTableWarning(hint *ast.TableOptimizerHint) { + var sb strings.Builder + ctx := format.NewRestoreCtx(0, &sb) + if err := hint.Restore(ctx); err != nil { + return + } + errMsg := fmt.Sprintf("Hint %s is inapplicable. Please specify the table names in the arguments.", sb.String()) + b.ctx.GetSessionVars().StmtCtx.AppendWarning(ErrInternal.GenWithStack(errMsg)) +} + func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType nodeType, currentLevel int) { hints = b.hintProcessor.getCurrentStmtHints(hints, nodeType, currentLevel) var ( @@ -2196,15 +2206,35 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n for _, hint := range hints { switch hint.HintName.L { case TiDBMergeJoin, HintSMJ: - sortMergeTables = append(sortMergeTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { + sortMergeTables = append(sortMergeTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + } case TiDBIndexNestedLoopJoin, HintINLJ: - INLJTables = append(INLJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { + INLJTables = append(INLJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + } case HintINLHJ: - INLHJTables = append(INLHJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { + INLHJTables = append(INLHJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + } case HintINLMJ: - INLMJTables = append(INLMJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { + INLMJTables = append(INLMJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + } case TiDBHashJoin, HintHJ: - hashJoinTables = append(hashJoinTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { + hashJoinTables = append(hashJoinTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + } case HintHashAgg: aggHints.preferAggType |= preferHashAgg case HintStreamAgg: @@ -2212,7 +2242,9 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n case HintAggToCop: aggHints.preferAggToCop = true case HintUseIndex: - if len(hint.Tables) != 0 { + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { dbName := hint.Tables[0].DBName if dbName.L == "" { dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) @@ -2228,7 +2260,9 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n }) } case HintIgnoreIndex: - if len(hint.Tables) != 0 { + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { dbName := hint.Tables[0].DBName if dbName.L == "" { dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) @@ -2251,7 +2285,9 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n tikvTables = tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel) } case HintIndexMerge: - if len(hint.Tables) != 0 { + if len(hint.Tables) == 0 { + b.pushHintWithoutTableWarning(hint) + } else { indexMergeHintList = append(indexMergeHintList, indexHintInfo{ tblName: hint.Tables[0].TableName, indexHint: &ast.IndexHint{ diff --git a/planner/core/testdata/integration_suite_in.json b/planner/core/testdata/integration_suite_in.json index 9825d1262b598..2a80667577fa1 100644 --- a/planner/core/testdata/integration_suite_in.json +++ b/planner/core/testdata/integration_suite_in.json @@ -93,5 +93,20 @@ "desc select /*+ TIDB_INLJ(t2)*/ * from t1, t2 where t1.a = t2.a and t1.b = t2.b", "desc select /*+ TIDB_INLJ(t2)*/ * from t1, t2 where t1.a = t2.a and t1.b = t2.a and t1.b = t2.b" ] + }, + { + "name": "TestHintWithoutTableWarning", + "cases": [ + "select /*+ TIDB_SMJ() */ * from t1, t2 where t1.a=t2.a", + "select /*+ MERGE_JOIN() */ * from t1, t2 where t1.a=t2.a", + "select /*+ INL_JOIN() */ * from t1, t2 where t1.a=t2.a", + "select /*+ TIDB_INLJ() */ * from t1, t2 where t1.a=t2.a", + "select /*+ INL_HASH_JOIN() */ * from t1, t2 where t1.a=t2.a", + "select /*+ INL_MERGE_JOIN() */ * from t1, t2 where t1.a=t2.a", + "select /*+ HASH_JOIN() */ * from t1, t2 where t1.a=t2.a", + "select /*+ USE_INDEX() */ * from t1, t2 where t1.a=t2.a", + "select /*+ IGNORE_INDEX() */ * from t1, t2 where t1.a=t2.a", + "select /*+ USE_INDEX_MERGE() */ * from t1, t2 where t1.a=t2.a" + ] } ] diff --git a/planner/core/testdata/integration_suite_out.json b/planner/core/testdata/integration_suite_out.json index f6cbb2ca7e59f..157fe8c06bcb1 100644 --- a/planner/core/testdata/integration_suite_out.json +++ b/planner/core/testdata/integration_suite_out.json @@ -389,5 +389,70 @@ ] } ] + }, + { + "Name": "TestHintWithoutTableWarning", + "Cases": [ + { + "SQL": "select /*+ TIDB_SMJ() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint TIDB_SMJ() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ MERGE_JOIN() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint MERGE_JOIN() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ INL_JOIN() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint INL_JOIN() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ TIDB_INLJ() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint TIDB_INLJ() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ INL_HASH_JOIN() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint INL_HASH_JOIN() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ INL_MERGE_JOIN() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint INL_MERGE_JOIN() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ HASH_JOIN() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[planner:1815]Hint HASH_JOIN() is inapplicable. Please specify the table names in the arguments." + ] + }, + { + "SQL": "select /*+ USE_INDEX() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[parser:1064]You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use [parser:8064]Optimizer hint syntax error at line 1 column 22 near \") */\" " + ] + }, + { + "SQL": "select /*+ IGNORE_INDEX() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[parser:1064]You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use [parser:8064]Optimizer hint syntax error at line 1 column 25 near \") */\" " + ] + }, + { + "SQL": "select /*+ USE_INDEX_MERGE() */ * from t1, t2 where t1.a=t2.a", + "Warnings": [ + "[parser:1064]You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use [parser:8064]Optimizer hint syntax error at line 1 column 28 near \") */\" " + ] + } + ] } ] From 1058d0547082628b24ca5ad154457e3b3fd0c486 Mon Sep 17 00:00:00 2001 From: Mingcong Han Date: Mon, 30 Mar 2020 13:26:22 +0800 Subject: [PATCH 2/2] address comments --- planner/core/logical_plan_builder.go | 118 +++++++++++---------------- 1 file changed, 49 insertions(+), 69 deletions(-) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index a56cfead94eaa..b0d4f47a5cf0c 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -2205,37 +2205,27 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n timeRangeHint ast.HintTimeRange ) for _, hint := range hints { + // Set warning for the hint that requires the table name. switch hint.HintName.L { - case TiDBMergeJoin, HintSMJ: + case TiDBMergeJoin, HintSMJ, TiDBIndexNestedLoopJoin, HintINLJ, HintINLHJ, HintINLMJ, + TiDBHashJoin, HintHJ, HintUseIndex, HintIgnoreIndex, HintIndexMerge: if len(hint.Tables) == 0 { b.pushHintWithoutTableWarning(hint) - } else { - sortMergeTables = append(sortMergeTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) + continue } + } + + switch hint.HintName.L { + case TiDBMergeJoin, HintSMJ: + sortMergeTables = append(sortMergeTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) case TiDBIndexNestedLoopJoin, HintINLJ: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - INLJTables = append(INLJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) - } + INLJTables = append(INLJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) case HintINLHJ: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - INLHJTables = append(INLHJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) - } + INLHJTables = append(INLHJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) case HintINLMJ: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - INLMJTables = append(INLMJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) - } + INLMJTables = append(INLMJTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) case TiDBHashJoin, HintHJ: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - hashJoinTables = append(hashJoinTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) - } + hashJoinTables = append(hashJoinTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) case HintHashAgg: aggHints.preferAggType |= preferHashAgg case HintStreamAgg: @@ -2243,41 +2233,33 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n case HintAggToCop: aggHints.preferAggToCop = true case HintUseIndex: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - dbName := hint.Tables[0].DBName - if dbName.L == "" { - dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) - } - indexHintList = append(indexHintList, indexHintInfo{ - dbName: dbName, - tblName: hint.Tables[0].TableName, - indexHint: &ast.IndexHint{ - IndexNames: hint.Indexes, - HintType: ast.HintUse, - HintScope: ast.HintForScan, - }, - }) + dbName := hint.Tables[0].DBName + if dbName.L == "" { + dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) } + indexHintList = append(indexHintList, indexHintInfo{ + dbName: dbName, + tblName: hint.Tables[0].TableName, + indexHint: &ast.IndexHint{ + IndexNames: hint.Indexes, + HintType: ast.HintUse, + HintScope: ast.HintForScan, + }, + }) case HintIgnoreIndex: - if len(hint.Tables) == 0 { - b.pushHintWithoutTableWarning(hint) - } else { - dbName := hint.Tables[0].DBName - if dbName.L == "" { - dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) - } - indexHintList = append(indexHintList, indexHintInfo{ - dbName: dbName, - tblName: hint.Tables[0].TableName, - indexHint: &ast.IndexHint{ - IndexNames: hint.Indexes, - HintType: ast.HintIgnore, - HintScope: ast.HintForScan, - }, - }) + dbName := hint.Tables[0].DBName + if dbName.L == "" { + dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) } + indexHintList = append(indexHintList, indexHintInfo{ + dbName: dbName, + tblName: hint.Tables[0].TableName, + indexHint: &ast.IndexHint{ + IndexNames: hint.Indexes, + HintType: ast.HintIgnore, + HintScope: ast.HintForScan, + }, + }) case HintReadFromStorage: switch hint.HintData.(model.CIStr).L { case HintTiFlash: @@ -2286,21 +2268,19 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n tikvTables = append(tikvTables, tableNames2HintTableInfo(b.ctx, hint.Tables, b.hintProcessor, nodeType, currentLevel)...) } case HintIndexMerge: - if len(hint.Tables) != 0 { - dbName := hint.Tables[0].DBName - if dbName.L == "" { - dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) - } - indexMergeHintList = append(indexMergeHintList, indexHintInfo{ - dbName: dbName, - tblName: hint.Tables[0].TableName, - indexHint: &ast.IndexHint{ - IndexNames: hint.Indexes, - HintType: ast.HintUse, - HintScope: ast.HintForScan, - }, - }) + dbName := hint.Tables[0].DBName + if dbName.L == "" { + dbName = model.NewCIStr(b.ctx.GetSessionVars().CurrentDB) } + indexMergeHintList = append(indexMergeHintList, indexHintInfo{ + dbName: dbName, + tblName: hint.Tables[0].TableName, + indexHint: &ast.IndexHint{ + IndexNames: hint.Indexes, + HintType: ast.HintUse, + HintScope: ast.HintForScan, + }, + }) case HintTimeRange: timeRangeHint = hint.HintData.(ast.HintTimeRange) default: