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: do not choose point get when exists read from tiflash hint. #17046

Closed
Closed
Show file tree
Hide file tree
Changes from 5 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
4 changes: 2 additions & 2 deletions planner/core/find_best_task.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty) (t task, err

for _, candidate := range candidates {
path := candidate.path
if path.PartialIndexPaths != nil {
if path.PartialIndexPaths != nil && ds.preferStoreType&preferTiFlash == 0 {
idxMergeTask, err := ds.convertToIndexMergeScan(prop, candidate)
if err != nil {
return nil, err
Expand All @@ -510,7 +510,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty) (t task, err
}, nil
}
canConvertPointGet := (!ds.isPartition && len(path.Ranges) > 0) || (ds.isPartition && len(path.Ranges) == 1)
canConvertPointGet = canConvertPointGet && candidate.path.StoreType != kv.TiFlash
canConvertPointGet = canConvertPointGet && candidate.path.StoreType != kv.TiFlash && ds.preferStoreType&preferTiFlash == 0
if !candidate.path.IsTablePath {
canConvertPointGet = canConvertPointGet &&
candidate.path.Index.Unique &&
Expand Down
35 changes: 35 additions & 0 deletions planner/core/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,41 @@ func (s *testIntegrationSerialSuite) TestIsolationReadTiFlashNotChoosePointGet(c
}
}

func (s *testIntegrationSerialSuite) TestHintReadTiFlashNotChoosePointGet(c *C) {
tk := testkit.NewTestKit(c, s.store)

tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a int, b int, primary key (a))")
tk.MustExec("insert into t values(1,1), (2,2)")

// Create virtual tiflash replica info.
dom := domain.GetDomain(tk.Se)
is := dom.InfoSchema()
db, exists := is.SchemaByName(model.NewCIStr("test"))
c.Assert(exists, IsTrue)
for _, tblInfo := range db.Tables {
tblInfo.TiFlashReplica = &model.TiFlashReplicaInfo{
Count: 1,
Available: true,
}
}

var input []string
var output []struct {
SQL string
Result []string
}
s.testData.GetTestCases(c, &input, &output)
for i, tt := range input {
s.testData.OnRecord(func() {
output[i].SQL = tt
output[i].Result = s.testData.ConvertRowsToStrings(tk.MustQuery(tt).Rows())
})
tk.MustQuery(tt).Check(testkit.Rows(output[i].Result...))
}
}

func (s *testIntegrationSerialSuite) TestIsolationReadTiFlashUseIndexHint(c *C) {
tk := testkit.NewTestKit(c, s.store)

Expand Down
19 changes: 19 additions & 0 deletions planner/core/point_get_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,23 @@ func newBatchPointGetPlan(
}.Init(ctx, statsInfo, schema, names, 0)
}

func isHintReadFromTiFlash(hint *ast.TableOptimizerHint) bool {
return hint.HintName.L == HintReadFromStorage && hint.HintData.(model.CIStr).L == HintTiFlash
}

func selStmtHasTiFlashHint(selStmt *ast.SelectStmt) bool {
for _, hint := range selStmt.TableHints {
if isHintReadFromTiFlash(hint) {
return true
}
}
return false
}

func tryWhereIn2BatchPointGet(ctx sessionctx.Context, selStmt *ast.SelectStmt) *BatchPointGetPlan {
if selStmtHasTiFlashHint(selStmt) {
return nil
}
if selStmt.OrderBy != nil || selStmt.GroupBy != nil ||
selStmt.Limit != nil || selStmt.Having != nil ||
len(selStmt.WindowSpecs) > 0 {
Expand Down Expand Up @@ -636,6 +652,9 @@ func tryWhereIn2BatchPointGet(ctx sessionctx.Context, selStmt *ast.SelectStmt) *
// 3. All the columns must be public and generated.
// 4. The condition is an access path that the range is a unique key.
func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt) *PointGetPlan {
if selStmtHasTiFlashHint(selStmt) {
return nil
}
if selStmt.Having != nil {
return nil
} else if selStmt.Limit != nil {
Expand Down
22 changes: 21 additions & 1 deletion planner/core/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/pingcap/parser/ast"
"github.com/pingcap/parser/mysql"
"github.com/pingcap/tidb/expression"
"github.com/pingcap/tidb/kv"
"github.com/pingcap/tidb/planner/property"
"github.com/pingcap/tidb/planner/util"
"github.com/pingcap/tidb/statistics"
Expand Down Expand Up @@ -182,8 +183,12 @@ func (ds *DataSource) DeriveStats(childStats []*property.StatsInfo, selfSchema *
for i, expr := range ds.pushedDownConds {
ds.pushedDownConds[i] = expression.PushDownNot(ds.ctx, expr)
}
var tiflashPath *util.AccessPath
for _, path := range ds.possibleAccessPaths {
if path.IsTablePath {
if path.StoreType == kv.TiFlash {
tiflashPath = path
}
continue
}
err := ds.fillIndexPath(path, ds.pushedDownConds)
Expand All @@ -192,16 +197,28 @@ func (ds *DataSource) DeriveStats(childStats []*property.StatsInfo, selfSchema *
}
}
ds.stats = ds.deriveStatsByFilter(ds.pushedDownConds, ds.possibleAccessPaths)
if tiflashPath != nil {
_, err := ds.deriveTablePathStats(tiflashPath, ds.pushedDownConds, false)
if err != nil {
return nil, err
}
}
for _, path := range ds.possibleAccessPaths {
if path.StoreType == kv.TiFlash {
continue
}
if path.IsTablePath {
noIntervalRanges, err := ds.deriveTablePathStats(path, ds.pushedDownConds, false)
if err != nil {
return nil, err
}
// If we have point or empty range, just remove other possible paths.
if noIntervalRanges || len(path.Ranges) == 0 {
if (noIntervalRanges || len(path.Ranges) == 0) && path.StoreType == kv.TiKV {
ds.possibleAccessPaths[0] = path
ds.possibleAccessPaths = ds.possibleAccessPaths[:1]
if tiflashPath != nil {
ds.possibleAccessPaths = append(ds.possibleAccessPaths, tiflashPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we keep the tiflash path here? would the behavior be inconsistent with other hints like USE_INDEX? since those indexes are removed as well ignoring potential hints.

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'm afraid it will affect other index hints. Since the path has been filtered.

}
break
}
continue
Expand All @@ -211,6 +228,9 @@ func (ds *DataSource) DeriveStats(childStats []*property.StatsInfo, selfSchema *
if (noIntervalRanges && path.Index.Unique) || len(path.Ranges) == 0 {
ds.possibleAccessPaths[0] = path
ds.possibleAccessPaths = ds.possibleAccessPaths[:1]
if tiflashPath != nil {
ds.possibleAccessPaths = append(ds.possibleAccessPaths, tiflashPath)
}
break
}
}
Expand Down
8 changes: 8 additions & 0 deletions planner/core/testdata/integration_serial_suite_in.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@
"explain select * from t where t.a in (1, 2)"
]
},
{
"name": "TestHintReadTiFlashNotChoosePointGet",
"cases": [
"explain select /*+ read_from_storage(tiflash[t]) */ * from t where t.a = 1",
"explain select /*+ read_from_storage(tiflash[t]) */ * from t where t.a in (1, 2, 3)",
"explain select /*+ read_from_storage(tikv[t]) */ * from t where t.a = 1"
]
},
{
"name": "TestIsolationReadTiFlashUseIndexHint",
"cases": [
Expand Down
25 changes: 25 additions & 0 deletions planner/core/testdata/integration_serial_suite_out.json
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,31 @@
}
]
},
{
"Name": "TestHintReadTiFlashNotChoosePointGet",
"Cases": [
{
"SQL": "explain select /*+ read_from_storage(tiflash[t]) */ * from t where t.a = 1",
"Result": [
"TableReader_6 1.00 root data:TableRangeScan_5",
"└─TableRangeScan_5 1.00 cop[tiflash] table:t range:[1,1], keep order:false, stats:pseudo"
]
},
{
"SQL": "explain select /*+ read_from_storage(tiflash[t]) */ * from t where t.a in (1, 2, 3)",
"Result": [
"TableReader_6 3.00 root data:TableRangeScan_5",
"└─TableRangeScan_5 3.00 cop[tiflash] table:t range:[1,1], [2,2], [3,3], keep order:false, stats:pseudo"
]
},
{
"SQL": "explain select /*+ read_from_storage(tikv[t]) */ * from t where t.a = 1",
"Result": [
"Point_Get_1 1.00 root table:t handle:1"
]
}
]
},
{
"Name": "TestIsolationReadTiFlashUseIndexHint",
"Cases": [
Expand Down