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 should not use point get plan #21124

Merged
merged 13 commits into from
Nov 24, 2020
Merged
49 changes: 49 additions & 0 deletions planner/core/point_get_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/pingcap/parser/mysql"
"github.com/pingcap/parser/opcode"
"github.com/pingcap/parser/terror"
ptypes "github.com/pingcap/parser/types"
"github.com/pingcap/tidb/expression"
"github.com/pingcap/tidb/infoschema"
"github.com/pingcap/tidb/kv"
Expand Down Expand Up @@ -518,6 +519,9 @@ func newBatchPointGetPlan(
if d.IsNull() {
return nil
}
if !checkCanConvertInPointGet(handleCol, d) {
return nil
}
intDatum, err := d.ConvertTo(ctx.GetSessionVars().StmtCtx, &handleCol.FieldType)
if err != nil {
return nil
Expand All @@ -540,6 +544,14 @@ func newBatchPointGetPlan(
// The columns in where clause should be covered by unique index
var matchIdxInfo *model.IndexInfo
permutations := make([]int, len(whereColNames))
colInfos := make([]*model.ColumnInfo, len(whereColNames))
for i, innerCol := range whereColNames {
for _, col := range tbl.Columns {
if col.Name.L == innerCol {
colInfos[i] = col
}
}
}
for _, idxInfo := range tbl.Indices {
if !idxInfo.Unique || idxInfo.State != model.StatePublic || idxInfo.Invisible {
continue
Expand Down Expand Up @@ -592,17 +604,29 @@ func newBatchPointGetPlan(
permIndex := permutations[index]
switch innerX := inner.(type) {
case *driver.ValueExpr:
if !checkCanConvertInPointGet(colInfos[index], innerX.Datum) {
return nil
}
values[permIndex] = innerX.Datum
case *driver.ParamMarkerExpr:
if !checkCanConvertInPointGet(colInfos[index], innerX.Datum) {
return nil
}
values[permIndex] = innerX.Datum
valuesParams[permIndex] = innerX
default:
return nil
}
}
case *driver.ValueExpr:
if !checkCanConvertInPointGet(colInfos[0], x.Datum) {
return nil
}
values = []types.Datum{x.Datum}
case *driver.ParamMarkerExpr:
if !checkCanConvertInPointGet(colInfos[0], x.Datum) {
return nil
}
values = []types.Datum{x.Datum}
valuesParams = []*driver.ParamMarkerExpr{x}
default:
Expand Down Expand Up @@ -1017,6 +1041,9 @@ func getNameValuePairs(stmtCtx *stmtctx.StatementContext, tbl *model.TableInfo,
(col.Tp == mysql.TypeString && col.Collate == charset.CollationBin) { // This type we needn't to pad `\0` in here.
return append(nvPairs, nameValuePair{colName: colName.Name.Name.L, value: d, param: param}), false
}
if !checkCanConvertInPointGet(col, d) {
return nil, false
}
dVal, err := d.ConvertTo(stmtCtx, &col.FieldType)
if err != nil {
if terror.ErrorEqual(types.ErrOverflow, err) {
Expand All @@ -1040,6 +1067,28 @@ func getNameValuePairs(stmtCtx *stmtctx.StatementContext, tbl *model.TableInfo,
return nil, false
}

func checkCanConvertInPointGet(col *model.ColumnInfo, d types.Datum) bool {
kind := d.Kind()
switch col.FieldType.EvalType() {
case ptypes.ETString:
switch kind {
case types.KindInt64, types.KindUint64,
types.KindFloat32, types.KindFloat64, types.KindMysqlDecimal:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should bit be contained here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do bit type test on mysql later.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, please also test year and other int like types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

datetime serise is tested, no problem now. do we need test case for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's much better. More test cases of different types could help us avoid this problem in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzmhhh123 bit and Hex binary literal tested same as MySQL, so just char column with numeric literal need to be reject from try point get plan.

// column type is String and constant type is numeric
return false
}
}
switch col.FieldType.Tp {
case mysql.TypeBit:
switch kind {
case types.KindString:
// column type is Bit and constant type is string
return false
}
}
return true
}

func findPKHandle(tblInfo *model.TableInfo, pairs []nameValuePair) (handlePair nameValuePair, fieldType *types.FieldType) {
if !tblInfo.PKIsHandle {
rowIDIdx := findInPairs("_tidb_rowid", pairs)
Expand Down
36 changes: 36 additions & 0 deletions planner/core/point_get_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ func (s *testPointGetSuite) TestIssue19141(c *C) {
func (s *testPointGetSuite) TestSelectInMultiColumns(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("drop table if exists t2")
tk.MustExec("create table t2(a int, b int, c int, primary key(a, b, c));")
tk.MustExec("insert into t2 values (1, 1, 1), (2, 2, 2), (3, 3, 3), (4, 4, 4)")
tk.MustQuery("select * from t2 where (a, b, c) in ((1, 1, 1));").Check(testkit.Rows("1 1 1"))
Expand Down Expand Up @@ -562,3 +563,38 @@ func (s *testPointGetSuite) TestBatchPointGetWithInvisibleIndex(c *C) {
" └─TableFullScan_5 10000.00 cop[tikv] table:t keep order:false, stats:pseudo",
))
}

func (s *testPointGetSuite) TestCBOShouldNotUsePointGet(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("drop tables if exists t1, t2, t3, t4, t5")
tk.MustExec("create table t1(id varchar(20) primary key)")
tk.MustExec("create table t2(id varchar(20), unique(id))")
tk.MustExec("create table t3(id varchar(20), d varchar(20), unique(id, d))")
tk.MustExec("create table t4(id int, d varchar(20), c varchar(20), unique(id, d))")
tk.MustExec("create table t5(id bit(64) primary key)")
tk.MustExec("insert into t1 values('asdf'), ('1asdf')")
tk.MustExec("insert into t2 values('asdf'), ('1asdf')")
tk.MustExec("insert into t3 values('asdf', 't'), ('1asdf', 't')")
tk.MustExec("insert into t4 values(1, 'b', 'asdf'), (1, 'c', 'jkl'), (1, 'd', '1jkl')")
tk.MustExec("insert into t5 values(48)")

var input []string
var output []struct {
SQL string
Plan []string
Res []string
}
s.testData.GetTestCases(c, &input, &output)
for i, sql := range input {
plan := tk.MustQuery("explain " + sql)
res := tk.MustQuery(sql)
s.testData.OnRecord(func() {
output[i].SQL = sql
output[i].Plan = s.testData.ConvertRowsToStrings(plan.Rows())
output[i].Res = s.testData.ConvertRowsToStrings(res.Rows())
})
plan.Check(testkit.Rows(output[i].Plan...))
res.Check(testkit.Rows(output[i].Res...))
}
}
17 changes: 17 additions & 0 deletions planner/core/testdata/point_get_plan_in.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,22 @@
"select * from t t1 join t t2 on t1.a = t2.a where t1.a = '4' and (t2.b, t2.c) in ((1,1),(2,2))",
"select * from t where (t.b, t.c) in ((2,2), (3,3), (4,4)) order by t.b, t.c"
]
},
{
"name": "TestCBOShouldNotUsePointGet",
"cases": [
"select * from t1 where id = 0",
"select * from t1 where id = x'00'",
"select * from t1 where id = b'00'",
"select * from t1 where id = 0.0",
"select * from t1 where id = 1.0",
"select * from t1 where id in (0, 1)",
"select * from t2 where id = 0",
"select * from t2 where id in (0, 1)",
"select * from t3 where (id, d) in ((0, 't'), (1, 't'))",
"select * from t4 where (id, d, c) in ((1, 'b', 0))",
"select * from t4 where (id, d, c) in ((1, 0, 0))",
"select * from t5 where id in ('0')"
]
}
]
132 changes: 132 additions & 0 deletions planner/core/testdata/point_get_plan_out.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,137 @@
]
}
]
},
{
"Name": "TestCBOShouldNotUsePointGet",
"Cases": [
{
"SQL": "select * from t1 where id = 0",
"Plan": [
"TableReader_7 8000.00 root data:Selection_6",
"└─Selection_6 8000.00 cop[tikv] eq(cast(test.t1.id), 0)",
" └─TableFullScan_5 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo"
],
"Res": [
"asdf"
]
},
{
"SQL": "select * from t1 where id = x'00'",
"Plan": [
"Point_Get_1 1.00 root table:t1, clustered index:PRIMARY(id) "
],
"Res": []
},
{
"SQL": "select * from t1 where id = b'00'",
"Plan": [
"Point_Get_1 1.00 root table:t1, clustered index:PRIMARY(id) "
],
"Res": []
},
{
"SQL": "select * from t1 where id = 0.0",
"Plan": [
"TableReader_7 8000.00 root data:Selection_6",
"└─Selection_6 8000.00 cop[tikv] eq(cast(test.t1.id), 0)",
" └─TableFullScan_5 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo"
],
"Res": [
"asdf"
]
},
{
"SQL": "select * from t1 where id = 1.0",
"Plan": [
"TableReader_7 8000.00 root data:Selection_6",
"└─Selection_6 8000.00 cop[tikv] eq(cast(test.t1.id), 1)",
" └─TableFullScan_5 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo"
],
"Res": [
"1asdf"
]
},
{
"SQL": "select * from t1 where id in (0, 1)",
"Plan": [
"TableReader_7 9600.00 root data:Selection_6",
"└─Selection_6 9600.00 cop[tikv] or(eq(cast(test.t1.id), 0), eq(cast(test.t1.id), 1))",
" └─TableFullScan_5 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo"
],
"Res": [
"1asdf",
"asdf"
]
},
{
"SQL": "select * from t2 where id = 0",
"Plan": [
"IndexReader_10 8000.00 root index:Selection_9",
"└─Selection_9 8000.00 cop[tikv] eq(cast(test.t2.id), 0)",
" └─IndexFullScan_8 10000.00 cop[tikv] table:t2, index:id(id) keep order:false, stats:pseudo"
],
"Res": [
"asdf"
]
},
{
"SQL": "select * from t2 where id in (0, 1)",
"Plan": [
"IndexReader_10 9600.00 root index:Selection_9",
"└─Selection_9 9600.00 cop[tikv] or(eq(cast(test.t2.id), 0), eq(cast(test.t2.id), 1))",
" └─IndexFullScan_8 10000.00 cop[tikv] table:t2, index:id(id) keep order:false, stats:pseudo"
],
"Res": [
"1asdf",
"asdf"
]
},
{
"SQL": "select * from t3 where (id, d) in ((0, 't'), (1, 't'))",
"Plan": [
"IndexReader_10 15.99 root index:Selection_9",
"└─Selection_9 15.99 cop[tikv] or(and(eq(cast(test.t3.id), 0), eq(test.t3.d, \"t\")), and(eq(cast(test.t3.id), 1), eq(test.t3.d, \"t\")))",
" └─IndexFullScan_8 10000.00 cop[tikv] table:t3, index:id(id, d) keep order:false, stats:pseudo"
],
"Res": [
"1asdf t",
"asdf t"
]
},
{
"SQL": "select * from t4 where (id, d, c) in ((1, 'b', 0))",
"Plan": [
"Selection_6 0.80 root eq(cast(test.t4.c), 0)",
"└─Point_Get_5 1.00 root table:t4, index:id(id, d) "
],
"Res": [
"1 b asdf"
]
},
{
"SQL": "select * from t4 where (id, d, c) in ((1, 0, 0))",
"Plan": [
"IndexLookUp_12 8.00 root ",
"├─Selection_10(Build) 8.00 cop[tikv] eq(cast(test.t4.d), 0)",
"│ └─IndexRangeScan_8 10.00 cop[tikv] table:t4, index:id(id, d) range:[1,1], keep order:false, stats:pseudo",
"└─Selection_11(Probe) 8.00 cop[tikv] eq(cast(test.t4.c), 0)",
" └─TableRowIDScan_9 8.00 cop[tikv] table:t4 keep order:false, stats:pseudo"
],
"Res": [
"1 b asdf",
"1 c jkl"
]
},
{
"SQL": "select * from t5 where id in ('0')",
"Plan": [
"Selection_5 8000.00 root eq(test.t5.id, 0)",
"└─TableReader_7 10000.00 root data:TableFullScan_6",
" └─TableFullScan_6 10000.00 cop[tikv] table:t5 keep order:false, stats:pseudo"
],
"Res": []
}
]
}
]