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: add missing CanExprsPushDown checks when generating IndexMerge path for or #41361

Merged
merged 1 commit into from
Feb 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 29 additions & 8 deletions planner/core/indexmerge_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,28 @@ func (ds *DataSource) generateIndexMergeOrPaths(filters []expression.Expression)
if !ok || sf.FuncName.L != ast.LogicOr {
continue
}
// shouldKeepCurrentFilter means the partial paths can't cover the current filter completely, so we must add
// the current filter into a Selection after partial paths.
shouldKeepCurrentFilter := false
var partialPaths = make([]*util.AccessPath, 0, usedIndexCount)
dnfItems := expression.FlattenDNFConditions(sf)
for _, item := range dnfItems {
cnfItems := expression.SplitCNFItems(item)
itemPaths := ds.accessPathsForConds(cnfItems, usedIndexCount)

pushedDownCNFItems := make([]expression.Expression, 0, len(cnfItems))
for _, cnfItem := range cnfItems {
if expression.CanExprsPushDown(ds.ctx.GetSessionVars().StmtCtx,
[]expression.Expression{cnfItem},
ds.ctx.GetClient(),
kv.TiKV,
time-and-fate marked this conversation as resolved.
Show resolved Hide resolved
) {
pushedDownCNFItems = append(pushedDownCNFItems, cnfItem)
} else {
shouldKeepCurrentFilter = true
}
}

itemPaths := ds.accessPathsForConds(pushedDownCNFItems, usedIndexCount)
if len(itemPaths) == 0 {
partialPaths = nil
break
Expand All @@ -140,7 +157,7 @@ func (ds *DataSource) generateIndexMergeOrPaths(filters []expression.Expression)
continue
}
if len(partialPaths) > 1 {
possiblePath := ds.buildIndexMergeOrPath(filters, partialPaths, i)
possiblePath := ds.buildIndexMergeOrPath(filters, partialPaths, i, shouldKeepCurrentFilter)
if possiblePath == nil {
return nil
}
Expand Down Expand Up @@ -308,28 +325,32 @@ func (ds *DataSource) buildIndexMergePartialPath(indexAccessPaths []*util.Access
}

// buildIndexMergeOrPath generates one possible IndexMergePath.
func (ds *DataSource) buildIndexMergeOrPath(filters []expression.Expression, partialPaths []*util.AccessPath, current int) *util.AccessPath {
func (ds *DataSource) buildIndexMergeOrPath(
filters []expression.Expression,
partialPaths []*util.AccessPath,
current int,
shouldKeepCurrentFilter bool,
) *util.AccessPath {
indexMergePath := &util.AccessPath{PartialIndexPaths: partialPaths}
indexMergePath.TableFilters = append(indexMergePath.TableFilters, filters[:current]...)
indexMergePath.TableFilters = append(indexMergePath.TableFilters, filters[current+1:]...)
var addCurrentFilter bool
for _, path := range partialPaths {
// If any partial path contains table filters, we need to keep the whole DNF filter in the Selection.
if len(path.TableFilters) > 0 {
addCurrentFilter = true
shouldKeepCurrentFilter = true
}
// If any partial path's index filter cannot be pushed to TiKV, we should keep the whole DNF filter.
if len(path.IndexFilters) != 0 && !expression.CanExprsPushDown(ds.ctx.GetSessionVars().StmtCtx, path.IndexFilters, ds.ctx.GetClient(), kv.TiKV) {
addCurrentFilter = true
shouldKeepCurrentFilter = true
// Clear IndexFilter, the whole filter will be put in indexMergePath.TableFilters.
path.IndexFilters = nil
}
if len(path.TableFilters) != 0 && !expression.CanExprsPushDown(ds.ctx.GetSessionVars().StmtCtx, path.TableFilters, ds.ctx.GetClient(), kv.TiKV) {
addCurrentFilter = true
shouldKeepCurrentFilter = true
path.TableFilters = nil
}
}
if addCurrentFilter {
if shouldKeepCurrentFilter {
indexMergePath.TableFilters = append(indexMergePath.TableFilters, filters[current])
}
return indexMergePath
Expand Down
21 changes: 21 additions & 0 deletions planner/core/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8431,3 +8431,24 @@ func TestIssue40285(t *testing.T) {
tk.MustExec("CREATE TABLE t(col1 enum('p5', '9a33x') NOT NULL DEFAULT 'p5',col2 tinyblob DEFAULT NULL) ENGINE = InnoDB DEFAULT CHARSET = latin1 COLLATE = latin1_bin;")
tk.MustQuery("(select last_value(col1) over () as r0 from t) union all (select col2 as r0 from t);")
}

// https://github.com/pingcap/tidb/issues/41273
func TestIssue41273(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec(`CREATE TABLE t (
a set('nwbk','r5','1ad3u','van','ir1z','y','9m','f1','z','e6yd','wfev') NOT NULL DEFAULT 'ir1z,f1,e6yd',
b enum('soo2','4s4j','qi9om','8ue','i71o','qon','3','3feh','6o1i','5yebx','d') NOT NULL DEFAULT '8ue',
c varchar(66) DEFAULT '13mdezixgcn',
PRIMARY KEY (a,b) /*T![clustered_index] CLUSTERED */,
UNIQUE KEY ib(b),
KEY ia(a)
)ENGINE=InnoDB DEFAULT CHARSET=ascii COLLATE=ascii_bin;`)
tk.MustExec("INSERT INTO t VALUES('ir1z,f1,e6yd','i71o','13mdezixgcn'),('ir1z,f1,e6yd','d','13mdezixgcn'),('nwbk','8ue','13mdezixgcn');")
expectedRes := []string{"ir1z,f1,e6yd d 13mdezixgcn", "ir1z,f1,e6yd i71o 13mdezixgcn", "nwbk 8ue 13mdezixgcn"}
tk.MustQuery("select * from t where a between 'e6yd' and 'z' or b <> '8ue';").Sort().Check(testkit.Rows(expectedRes...))
tk.MustQuery("select /*+ use_index_merge(t) */ * from t where a between 'e6yd' and 'z' or b <> '8ue';").Sort().Check(testkit.Rows(expectedRes...))
// For now tidb doesn't support push set type to TiKV, and column a is a set type, so we shouldn't generate a IndexMerge path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need add some log or warning to indicate why cannot use index merge ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rest lgtm

Copy link
Member Author

Choose a reason for hiding this comment

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

It would print a general warning "IndexMerge is inapplicable" like other cases when we failed to generate an IndexMerge path.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be better if it could print the particular reason. But we also don't print the reason in other cases, so I think it's unnecessary to add it in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

require.False(t, tk.HasPlanForLastExecution("IndexMerge"))
}