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.

Sign up for GitHub

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

Jump to bottom

planner: add warning when join hint has no arguments #15583

Merged
merged 7 commits into from
Mar 30, 2020
Merged

planner: add warning when join hint has no arguments #15583

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b0b537b
throw warnings when no table names in join hints
francis0407 Mar 23, 2020
43e27bc
Merge branch 'master' into join_hint_warning
francis0407 Mar 24, 2020
748112f
Merge branch 'master' into join_hint_warning
francis0407 Mar 24, 2020
a947796
Merge branch 'master' of https://github.com/pingcap/tidb into join_hi…
francis0407 Mar 30, 2020
1058d05
address comments
francis0407 Mar 30, 2020
005711d
Merge remote-tracking branch 'origin/join_hint_warning' into join_hin…
francis0407 Mar 30, 2020
8bcb1ff
Merge branch 'master' into join_hint_warning
sre-bot Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions planner/core/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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])
}
}
}
52 changes: 44 additions & 8 deletions planner/core/logical_plan_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2183,6 +2183,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 (
Expand All @@ -2195,23 +2205,45 @@ func (b *PlanBuilder) pushTableHints(hints []*ast.TableOptimizerHint, nodeType n
for _, hint := range hints {
switch hint.HintName.L {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about check hint.Tables before the switch. Then we can reduce much redundant code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have merged these codes. PTAL

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:
aggHints.preferAggType |= preferStreamAgg
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)
Expand All @@ -2227,7 +2259,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)
Expand All @@ -2250,7 +2284,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{
Expand Down
15 changes: 15 additions & 0 deletions planner/core/testdata/integration_suite_in.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,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"
]
}
]
65 changes: 65 additions & 0 deletions planner/core/testdata/integration_suite_out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,5 +254,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 \") */\" "
]
}
]
}
]