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: regard NULL as point when accessing composite index #30244

Merged
merged 10 commits into from
Dec 1, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
2 changes: 2 additions & 0 deletions distsql/request_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,8 @@ func encodeIndexKey(sc *stmtctx.StatementContext, ran *ranger.Range) ([]byte, []
}
}

// NOTE: this is a hard-code operation to avoid wrong results when accessing unique index with NULL;
// Please see https://github.com/pingcap/tidb/issues/29650 for more details
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we remove the hard-code operation in the following PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I plan to remove this hard-code operation with the switch tidb_regard_null_as_point together a few months later if there is no new bug.

if hasNull {
// Append 0 to make unique-key range [null, null] to be a scan rather than point-get.
high = kv.Key(high).Next()
Expand Down
2 changes: 1 addition & 1 deletion planner/core/common_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,7 @@ func IsPointGetWithPKOrUniqueKeyByAutoCommit(ctx sessionctx.Context, p Plan) (bo
return indexScan.IsPointGetByUniqueKey(ctx), nil
case *PhysicalTableReader:
tableScan := v.TablePlans[0].(*PhysicalTableScan)
isPointRange := len(tableScan.Ranges) == 1 && tableScan.Ranges[0].IsPoint(ctx)
isPointRange := len(tableScan.Ranges) == 1 && tableScan.Ranges[0].IsPointNonNullable(ctx)
if !isPointRange {
return false, nil
}
Expand Down
3 changes: 2 additions & 1 deletion planner/core/find_best_task.go
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,8 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter
if canConvertPointGet {
allRangeIsPoint := true
for _, ran := range path.Ranges {
if !ran.IsPoint(ds.ctx) {
if !ran.IsPointNonNullable(ds.ctx) {
// unique indexes can have duplicated NULL rows so we cannot use PointGet if there is NULL
allRangeIsPoint = false
break
}
Expand Down
50 changes: 50 additions & 0 deletions planner/core/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4835,6 +4835,56 @@ func (s *testIntegrationSerialSuite) TestRejectSortForMPP(c *C) {
}
}

func (s *testIntegrationSerialSuite) TestRegardNULLAsPoint(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")

tk.MustExec("drop table if exists tpk")
tk.MustExec(`create table tuk (a int, b int, c int, unique key (a, b, c))`)
tk.MustExec(`create table tik (a int, b int, c int, key (a, b, c))`)
for _, va := range []string{"NULL", "1"} {
for _, vb := range []string{"NULL", "1"} {
for _, vc := range []string{"NULL", "1"} {
tk.MustExec(fmt.Sprintf(`insert into tuk values (%v, %v, %v)`, va, vb, vc))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is better to insert each value twice expect (1,1,1). In this way we can check whether it returns multiple rows (expected) or just a single row(wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

tk.MustExec(fmt.Sprintf(`insert into tik values (%v, %v, %v)`, va, vb, vc))
if va == "1" && vb == "1" && vc == "1" {
continue
}
// duplicated NULL rows
tk.MustExec(fmt.Sprintf(`insert into tuk values (%v, %v, %v)`, va, vb, vc))
tk.MustExec(fmt.Sprintf(`insert into tik values (%v, %v, %v)`, va, vb, vc))
}
}
}

var input []string
var output []struct {
SQL string
PlanEnabled []string
PlanDisabled []string
Result []string
}
s.testData.GetTestCases(c, &input, &output)
for i, tt := range input {
s.testData.OnRecord(func() {
output[i].SQL = tt
tk.MustExec(`set @@session.tidb_regard_null_as_point=true`)
output[i].PlanEnabled = s.testData.ConvertRowsToStrings(tk.MustQuery("explain " + tt).Rows())
output[i].Result = s.testData.ConvertRowsToStrings(tk.MustQuery(tt).Rows())

tk.MustExec(`set @@session.tidb_regard_null_as_point=false`)
output[i].PlanDisabled = s.testData.ConvertRowsToStrings(tk.MustQuery("explain " + tt).Rows())
})
tk.MustExec(`set @@session.tidb_regard_null_as_point=true`)
tk.MustQuery("explain " + tt).Check(testkit.Rows(output[i].PlanEnabled...))
tk.MustQuery(tt).Check(testkit.Rows(output[i].Result...))

tk.MustExec(`set @@session.tidb_regard_null_as_point=false`)
tk.MustQuery("explain " + tt).Check(testkit.Rows(output[i].PlanDisabled...))
tk.MustQuery(tt).Check(testkit.Rows(output[i].Result...))
}
}

func (s *testIntegrationSuite) TestIssues29711(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
Expand Down
3 changes: 3 additions & 0 deletions planner/core/partition_pruner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ func (s *testPartitionPruneSuit) TestListPartitionPruner(c *C) {
tk.MustExec("use test_partition")
tk.Se.GetSessionVars().EnableClusteredIndex = variable.ClusteredIndexDefModeIntOnly
tk.MustExec("set @@session.tidb_enable_list_partition = ON")
tk.MustExec(`set @@session.tidb_regard_null_as_point=false`)
tk.MustExec("create table t1 (id int, a int, b int ) partition by list ( a ) (partition p0 values in (1,2,3,4,5), partition p1 values in (6,7,8,9,10,null));")
tk.MustExec("create table t2 (a int, id int, b int) partition by list (a*3 + b - 2*a - b) (partition p0 values in (1,2,3,4,5), partition p1 values in (6,7,8,9,10,null));")
tk.MustExec("create table t3 (b int, id int, a int) partition by list columns (a) (partition p0 values in (1,2,3,4,5), partition p1 values in (6,7,8,9,10,null));")
Expand Down Expand Up @@ -169,6 +170,7 @@ func (s *testPartitionPruneSuit) TestListColumnsPartitionPruner(c *C) {
// tk1 use to test partition table with index.
tk1 := testkit.NewTestKit(c, s.store)
tk1.MustExec("drop database if exists test_partition_1;")
tk1.MustExec(`set @@session.tidb_regard_null_as_point=false`)
tk1.MustExec("create database test_partition_1")
tk1.MustExec("use test_partition_1")
tk1.MustExec("set @@session.tidb_enable_list_partition = ON")
Expand All @@ -180,6 +182,7 @@ func (s *testPartitionPruneSuit) TestListColumnsPartitionPruner(c *C) {
// tk2 use to compare the result with normal table.
tk2 := testkit.NewTestKit(c, s.store)
tk2.MustExec("drop database if exists test_partition_2;")
tk2.MustExec(`set @@session.tidb_regard_null_as_point=false`)
tk2.MustExec("create database test_partition_2")
tk2.MustExec("use test_partition_2")
tk2.MustExec("create table t1 (id int, a int, b int)")
Expand Down
2 changes: 1 addition & 1 deletion planner/core/physical_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -1201,7 +1201,7 @@ func (p *PhysicalIndexScan) IsPointGetByUniqueKey(sctx sessionctx.Context) bool
return len(p.Ranges) == 1 &&
p.Index.Unique &&
len(p.Ranges[0].LowVal) == len(p.Index.Columns) &&
p.Ranges[0].IsPoint(sctx)
p.Ranges[0].IsPointNonNullable(sctx)
}

// PhysicalSelection represents a filter.
Expand Down
12 changes: 12 additions & 0 deletions planner/core/plan_to_pb.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,18 @@ func checkCoverIndex(idx *model.IndexInfo, ranges []*ranger.Range) bool {
if len(rg.LowVal) != len(idx.Columns) {
return false
}
for _, v := range rg.LowVal {
if v.IsNull() {
// a unique index may have duplicated rows with NULLs, so we cannot set the unique attribute to true when the range has NULL
// please see https://github.com/pingcap/tidb/issues/29650 for more details
return false
}
}
for _, v := range rg.HighVal {
if v.IsNull() {
return false
}
}
}
return true
}
Expand Down
19 changes: 19 additions & 0 deletions planner/core/testdata/integration_serial_suite_in.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,25 @@
]

},
{
"name": "TestRegardNULLAsPoint",
"cases": [
"select * from tuk where a<=>null and b=1",
"select * from tik where a<=>null and b=1",
"select * from tuk where a<=>null and b>0 and b<2",
"select * from tik where a<=>null and b>0 and b<2",
"select * from tuk where a<=>null and b>=1 and b<2",
"select * from tik where a<=>null and b>=1 and b<2",
"select * from tuk where a<=>null and b=1 and c=1",
"select * from tik where a<=>null and b=1 and c=1",
"select * from tuk where a=1 and b<=>null and c=1",
"select * from tik where a=1 and b<=>null and c=1",
"select * from tuk where a<=>null and b<=>null and c=1",
"select * from tik where a<=>null and b<=>null and c=1",
"select * from tuk where a<=>null and b<=>null and c<=>null",
"select * from tik where a<=>null and b<=>null and c<=>null"
]
},
{
"name": "TestPushDownToTiFlashWithKeepOrder",
"cases": [
Expand Down
Loading