diff --git a/pkg/executor/builder.go b/pkg/executor/builder.go index 307da96b646cc..14688d2a78bf7 100644 --- a/pkg/executor/builder.go +++ b/pkg/executor/builder.go @@ -533,11 +533,16 @@ func buildIndexLookUpChecker(b *executorBuilder, p *physicalop.PhysicalIndexLook func (b *executorBuilder) buildCheckTable(v *plannercore.CheckTable) exec.Executor { canUseFastCheck := true + tblInfo := v.Table.Meta() for _, idx := range v.IndexInfos { if idx.MVIndex || idx.IsColumnarIndex() { canUseFastCheck = false break } + if idx.ContainsVirtualGeneratedTemporalWithDateColumn(tblInfo) { + canUseFastCheck = false + break + } for _, col := range idx.Columns { if col.Length != types.UnspecifiedLength { canUseFastCheck = false diff --git a/pkg/executor/check_table_index.go b/pkg/executor/check_table_index.go index 4055a762a8b36..1e3347f610614 100644 --- a/pkg/executor/check_table_index.go +++ b/pkg/executor/check_table_index.go @@ -896,6 +896,9 @@ func getCheckSum(ctx context.Context, se sessionctx.Context, sql string) ([]grou func getGlobalCheckSum(ctx context.Context, se sessionctx.Context, sql string) (checksum uint64, count int64, err error) { ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnAdmin) + // Create a new ExecDetails for the internal SQL to avoid data race with the parent statement. + // The parent context may carry an ExecDetails that is still being written to by concurrent goroutines. + ctx = execdetails.ContextWithInitializedExecDetails(ctx) rs, err := se.GetSQLExecutor().ExecuteInternal(ctx, sql) if err != nil { return 0, 0, err diff --git a/pkg/executor/test/admintest/admin_test.go b/pkg/executor/test/admintest/admin_test.go index e46f612743623..c66fe1c15db51 100644 --- a/pkg/executor/test/admintest/admin_test.go +++ b/pkg/executor/test/admintest/admin_test.go @@ -2293,6 +2293,7 @@ func TestFastAdminCheckWithError(t *testing.T) { // And the admin check shouldn't be blocked when meeting error. tk := testkit.NewTestKit(t, store) tk.MustExec("use test") + tk.MustExec("set tidb_enable_fast_table_check = 1") tk.MustExec("drop table if exists admin_test") tk.MustExec(` create table admin_test (c1 int, c2 int, @@ -2300,6 +2301,19 @@ func TestFastAdminCheckWithError(t *testing.T) { key idx6(c1), key idx7(c1), key idx8(c1), key idx9(c1), key idx10(c1)) `) tk.MustExecToErr("admin check table admin_test") + + tk.MustExec("drop table if exists admin_test_generated") + tk.MustExec(` + create table admin_test_generated ( + payload json, + f date as (JSON_EXTRACT(payload, '$.f')), + key idx_f(f) + ) + `) + tk.MustExec(`insert into admin_test_generated set payload='{"f":"2018-09-28"}'`) + // mockFastCheckTableError is still active here. This only succeeds because + // buildCheckTable falls back to CheckTableExec for idx_f instead of using FastCheckTableExec. + tk.MustExec("admin check table admin_test_generated") } func TestFastAdminCheckQuickPassSkipBucketed(t *testing.T) { @@ -2488,7 +2502,7 @@ func TestAdminCheckTableWithEnumAndPointGet(t *testing.T) { func TestFastCheckTableConcurrent(t *testing.T) { // This test verifies that concurrent execution of admin check table works correctly. // Note: The data race in ExecDetails (fixed by using ContextWithInitializedExecDetails - // in getCheckSum) cannot be detected in unit tests because mocktikv doesn't trigger + // in getCheckSum/getGlobalCheckSum) cannot be detected in unit tests because mocktikv doesn't trigger // the network traffic writes to ExecDetails that happen in real TiKV environments. store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) diff --git a/pkg/executor/test/tiflashtest/tiflash_test.go b/pkg/executor/test/tiflashtest/tiflash_test.go index 42a4581bbc6d2..b54e3916a1d5d 100644 --- a/pkg/executor/test/tiflashtest/tiflash_test.go +++ b/pkg/executor/test/tiflashtest/tiflash_test.go @@ -1811,9 +1811,9 @@ func TestMPP47766(t *testing.T) { tk.MustQuery("explain select date(test_time), count(1) as test_date from `traces` group by 1").Check(testkit.Rows( "Projection_4 8000.00 root test.traces.test_time_gen->Column#6, Column#5", "└─HashAgg_8 8000.00 root group by:test.traces.test_time_gen, funcs:count(1)->Column#5, funcs:firstrow(test.traces.test_time_gen)->test.traces.test_time_gen", - " └─TableReader_20 10000.00 root MppVersion: 3, data:ExchangeSender_19", - " └─ExchangeSender_19 10000.00 mpp[tiflash] ExchangeType: PassThrough", - " └─TableFullScan_18 10000.00 mpp[tiflash] table:traces keep order:false, stats:pseudo")) + " └─TableReader_18 10000.00 root MppVersion: 3, data:ExchangeSender_17", + " └─ExchangeSender_17 10000.00 mpp[tiflash] ExchangeType: PassThrough", + " └─TableFullScan_16 10000.00 mpp[tiflash] table:traces keep order:false, stats:pseudo")) tk.MustQuery("explain select /*+ read_from_storage(tiflash[traces]) */ date(test_time) as test_date, count(1) from `traces` group by 1"). Check(testkit.Rows( "TableReader_31 8000.00 root MppVersion: 3, data:ExchangeSender_30", diff --git a/pkg/importsdk/BUILD.bazel b/pkg/importsdk/BUILD.bazel index 3ce7a773851e9..ec05790001cca 100644 --- a/pkg/importsdk/BUILD.bazel +++ b/pkg/importsdk/BUILD.bazel @@ -54,6 +54,7 @@ go_test( "//pkg/lightning/config", "//pkg/lightning/log", "//pkg/lightning/mydump", + "//pkg/parser/ast", "//pkg/parser/mysql", "//pkg/util/table-filter", "@com_github_data_dog_go_sqlmock//:go-sqlmock", diff --git a/pkg/meta/model/index.go b/pkg/meta/model/index.go index 82e3a618291d3..46a91c6d1fc4f 100644 --- a/pkg/meta/model/index.go +++ b/pkg/meta/model/index.go @@ -25,6 +25,7 @@ import ( "github.com/pingcap/tidb/pkg/parser/mysql" "github.com/pingcap/tidb/pkg/parser/types" "github.com/pingcap/tidb/pkg/planner/cascades/base" + tidbtypes "github.com/pingcap/tidb/pkg/types" ) // DistanceMetric is the distance metric used by the vector index. @@ -365,6 +366,21 @@ func (index *IndexInfo) HasPrefixIndex() bool { return false } +// ContainsVirtualGeneratedTemporalWithDateColumn reports whether the index contains +// a virtual generated temporal-with-date column. +func (index *IndexInfo) ContainsVirtualGeneratedTemporalWithDateColumn(tblInfo *TableInfo) bool { + for _, idxCol := range index.Columns { + if idxCol.Offset < 0 || idxCol.Offset >= len(tblInfo.Columns) { + continue + } + col := tblInfo.Columns[idxCol.Offset] + if col.IsVirtualGenerated() && tidbtypes.IsTemporalWithDate(col.GetType()) { + return true + } + } + return false +} + // HasColumnInIndexColumns checks whether the index contains the column with the specified ID. func (index *IndexInfo) HasColumnInIndexColumns(tblInfo *TableInfo, colID int64) bool { for _, ic := range index.Columns { diff --git a/pkg/planner/core/casetest/index/BUILD.bazel b/pkg/planner/core/casetest/index/BUILD.bazel index fbf083085a332..74d84a123b655 100644 --- a/pkg/planner/core/casetest/index/BUILD.bazel +++ b/pkg/planner/core/casetest/index/BUILD.bazel @@ -9,7 +9,7 @@ go_test( ], data = glob(["testdata/**"]), flaky = True, - shard_count = 12, + shard_count = 13, deps = [ "//pkg/domain", "//pkg/domain/infosync", diff --git a/pkg/planner/core/casetest/index/index_test.go b/pkg/planner/core/casetest/index/index_test.go index 2122c8ff22d8f..62d87ef62970a 100644 --- a/pkg/planner/core/casetest/index/index_test.go +++ b/pkg/planner/core/casetest/index/index_test.go @@ -107,6 +107,44 @@ func TestInvisibleIndex(t *testing.T) { }) } +func TestVirtualGeneratedTemporalWithDateIndex(t *testing.T) { + testkit.RunTestUnderCascades(t, func(t *testing.T, tk *testkit.TestKit, cascades, caller string) { + tk.MustExec("use test") + tk.MustExec("drop table if exists t_issue_52520") + tk.MustExec("create table t_issue_52520 (a varchar(32), b date as (a), key idx_b(b))") + tk.MustExec("set @@sql_mode = ''") + tk.MustExec("insert into t_issue_52520(a) values ('2020-02-31')") + tk.MustExec("set @@sql_mode = 'ALLOW_INVALID_DATES'") + + tk.MustNoIndexUsed("select /* issue:52520 */ b from t_issue_52520 use index(idx_b)") + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + tk.MustQuery("select /* issue:52520 */ b from t_issue_52520 use index(idx_b)").Check(testkit.Rows("2020-02-31")) + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + + tk.MustNoIndexUsed("select /*+ USE_INDEX(t_issue_52520, idx_b) */ /* issue:52520 */ b from t_issue_52520") + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + tk.MustQuery("select /*+ USE_INDEX(t_issue_52520, idx_b) */ /* issue:52520 */ b from t_issue_52520").Check(testkit.Rows("2020-02-31")) + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + + tk.MustNoIndexUsed("select /* issue:52520 */ b from t_issue_52520 use index(idx_b) where b = '2020-02-31'") + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + tk.MustQuery("select /* issue:52520 */ b from t_issue_52520 use index(idx_b) where b = '2020-02-31'").Check(testkit.Rows("2020-02-31")) + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + + tk.MustNoIndexUsed("select /*+ USE_INDEX(t_issue_52520, idx_b) */ /* issue:52520 */ b from t_issue_52520 where b = '2020-02-31'") + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + tk.MustQuery("select /*+ USE_INDEX(t_issue_52520, idx_b) */ /* issue:52520 */ b from t_issue_52520 where b = '2020-02-31'").Check(testkit.Rows("2020-02-31")) + tk.MustQuery("show warnings").CheckContain("virtual generated temporal column") + + tk.MustNoIndexUsed("select /* issue:52520 */ b from t_issue_52520 ignore index(idx_b) where b = '2020-02-31'") + tk.MustQuery("show warnings").Check(testkit.Rows()) + + tk.MustExec("drop table if exists t_issue_52520_prefix") + tk.MustExec("create table t_issue_52520_prefix (a varchar(32), c int, b date as (a), key idx_safe(c), key idx_unsafe(b))") + tk.MustContainErrMsg("select /* issue:52520 */ c from t_issue_52520_prefix use index(idx) where c = 1", "Key 'idx' doesn't exist") + }) +} + func TestRangeDerivation(t *testing.T) { testkit.RunTestUnderCascades(t, func(t *testing.T, testKit *testkit.TestKit, cascades, caller string) { testKit.MustExec("use test") @@ -297,6 +335,14 @@ func TestInvertedIndex(t *testing.T) { testKit.MustNoIndexUsed("select * from t ignore index(idx_b) where b = 2") testKit.MustNoIndexUsed("select * from t ignore index(idx_c) where c = 3") testKit.MustNoIndexUsed("select * from t ignore index(idx_d) where d < 1") + + testKit.MustExec("drop table if exists t_issue_52520_columnar") + testKit.MustExec("create table t_issue_52520_columnar (a varchar(32), b date as (a))") + testKit.MustExec("alter table t_issue_52520_columnar set tiflash replica 1") + testKit.MustExec("alter table t_issue_52520_columnar add columnar index idx_b (b) using inverted") + testkit.SetTiFlashReplica(t, dom, "test", "t_issue_52520_columnar") + testKit.MustNoIndexUsed("select /* issue:52520 */ b from t_issue_52520_columnar use index(idx_b)") + testKit.MustQuery("show warnings").CheckContain("virtual generated temporal column") }) } diff --git a/pkg/planner/core/logical_plan_builder.go b/pkg/planner/core/logical_plan_builder.go index 439634b8b77a3..d70da13612548 100644 --- a/pkg/planner/core/logical_plan_builder.go +++ b/pkg/planner/core/logical_plan_builder.go @@ -5034,10 +5034,12 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as return nil, plannererrors.ErrPartitionClauseOnNonpartitioned } - possiblePaths, err := getPossibleAccessPaths(b.ctx, b.TableHints(), tn.IndexHints, tbl, dbName, tblName, b.isForUpdateRead, b.optFlag&rule.FlagPartitionProcessor > 0) + possiblePathInfo, err := getPossibleAccessPathInfo(b.ctx, b.TableHints(), tn.IndexHints, tbl, dbName, tblName, b.isForUpdateRead, b.optFlag&rule.FlagPartitionProcessor > 0) if err != nil { return nil, err } + possiblePaths := possiblePathInfo.paths + gcSubstituteExtraPaths := possiblePathInfo.gcSubstituteExtraPaths if tableInfo.IsView() { if tn.TableSample != nil { @@ -5091,6 +5093,10 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as if err != nil { return nil, err } + gcSubstituteExtraPaths, err = util.FilterPathByIsolationRead(b.ctx, gcSubstituteExtraPaths, tblName, dbName) + if err != nil && len(gcSubstituteExtraPaths) != 0 { + return nil, err + } } } @@ -5178,6 +5184,7 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as IndexMergeHints: indexMergeHints, PossibleAccessPaths: possiblePaths, AllPossibleAccessPaths: allPaths, + GcSubstituteExtraPaths: gcSubstituteExtraPaths, Columns: make([]*model.ColumnInfo, 0, countCnt), PartitionNames: tn.PartitionNames, TblCols: make([]*expression.Column, 0, countCnt), diff --git a/pkg/planner/core/operator/logicalop/logical_datasource.go b/pkg/planner/core/operator/logicalop/logical_datasource.go index 51a2a8d538b94..10aac3e27d7be 100644 --- a/pkg/planner/core/operator/logicalop/logical_datasource.go +++ b/pkg/planner/core/operator/logicalop/logical_datasource.go @@ -77,6 +77,9 @@ type DataSource struct { // AllPossibleAccessPaths stores all the possible access path from build datasource phase. AllPossibleAccessPaths []*util.AccessPath + // GcSubstituteExtraPaths stores filtered index paths that should still participate in + // generated-column substitution. They are not available for access-path selection. + GcSubstituteExtraPaths []*util.AccessPath // PossibleAccessPaths stores all the possible access path for one specific logical alternative. // because different logical alternative may have different filter condition, so the possible access path may be different. // like: diff --git a/pkg/planner/core/planbuilder.go b/pkg/planner/core/planbuilder.go index 94fad1d32ac4d..63c97357a23d3 100644 --- a/pkg/planner/core/planbuilder.go +++ b/pkg/planner/core/planbuilder.go @@ -1154,32 +1154,72 @@ func (*PlanBuilder) detectSelectWindow(sel *ast.SelectStmt) bool { return false } -func getPathByIndexName(paths []*util.AccessPath, idxName ast.CIStr, tblInfo *model.TableInfo) *util.AccessPath { - var indexPrefixPath *util.AccessPath - prefixMatches := 0 - for _, path := range paths { +type indexHintResolveStatus int + +const ( + indexHintResolveNotFound indexHintResolveStatus = iota + indexHintResolvePublicPath + indexHintResolveFilteredUnsafe +) + +func resolveIndexHintByName(publicPaths []*util.AccessPath, filteredUnsafeIndexes []*model.IndexInfo, idxName ast.CIStr, tblInfo *model.TableInfo) (*util.AccessPath, *model.IndexInfo, indexHintResolveStatus) { + // Prefer exact name matches over prefix matches. + // Check usable paths before filtered indexes. + for _, path := range publicPaths { // Only accept tikv's primary key table path. if path.IsTiKVTablePath() && isPrimaryIndex(idxName) && tblInfo.HasClusteredIndex() { - return path + return path, nil, indexHintResolvePublicPath } - // If it's not a tikv table path and the index is nil, it could not be any index path. if path.Index == nil { continue } if path.Index.Name.L == idxName.L { - return path + return path, nil, indexHintResolvePublicPath + } + } + // Check filtered unsafe indexes too, so hints on them can return "inapplicable" + // instead of being treated as missing indexes. + for _, index := range filteredUnsafeIndexes { + if index.Name.L == idxName.L { + return nil, index, indexHintResolveFilteredUnsafe + } + } + + var foundPath *util.AccessPath + var foundFilteredIndex *model.IndexInfo + prefixMatches := 0 + for _, path := range publicPaths { + if path.Index == nil { + continue } if strings.HasPrefix(path.Index.Name.L, idxName.L) { - indexPrefixPath = path + foundPath = path + foundFilteredIndex = nil prefixMatches++ } } - - // Return only unique prefix matches - if prefixMatches == 1 { - return indexPrefixPath + // Count filtered unsafe indexes in prefix matching too, so they participate in + // ambiguity detection with usable indexes. + for _, index := range filteredUnsafeIndexes { + if strings.HasPrefix(index.Name.L, idxName.L) { + foundPath = nil + foundFilteredIndex = index + prefixMatches++ + } } - return nil + // Return only unique prefix matches. + if prefixMatches != 1 { + return nil, nil, indexHintResolveNotFound + } + if foundFilteredIndex != nil { + // The hint uniquely points to a filtered unsafe index. + return nil, foundFilteredIndex, indexHintResolveFilteredUnsafe + } + return foundPath, nil, indexHintResolvePublicPath +} + +func genVirtualGeneratedTemporalWithDateIndexHintWarning(index *model.IndexInfo) string { + return fmt.Sprintf("hint is inapplicable, index %s contains a virtual generated temporal column whose values depend on sql_mode", index.Name.O) } func isPrimaryIndex(indexName ast.CIStr) bool { @@ -1272,6 +1312,19 @@ func checkAutoForceIndexLookUpPushDown(ctx base.PlanContext, tblInfo *model.Tabl } func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, indexHints []*ast.IndexHint, tbl table.Table, dbName, tblName ast.CIStr, check bool, hasFlagPartitionProcessor bool) ([]*util.AccessPath, error) { + info, err := getPossibleAccessPathInfo(ctx, tableHints, indexHints, tbl, dbName, tblName, check, hasFlagPartitionProcessor) + if err != nil { + return nil, err + } + return info.paths, nil +} + +type possibleAccessPathInfo struct { + paths []*util.AccessPath + gcSubstituteExtraPaths []*util.AccessPath +} + +func getPossibleAccessPathInfo(ctx base.PlanContext, tableHints *hint.PlanHints, indexHints []*ast.IndexHint, tbl table.Table, dbName, tblName ast.CIStr, check bool, hasFlagPartitionProcessor bool) (*possibleAccessPathInfo, error) { tblInfo := tbl.Meta() publicPaths := make([]*util.AccessPath, 0, len(tblInfo.Indices)+2) tp := kv.TiKV @@ -1308,6 +1361,8 @@ func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, in var err error // Inverted Index can not be used as access path index. invertedIndexes := make(map[string]struct{}) + var virtualGeneratedTemporalWithDateIndexes []*model.IndexInfo + filteredUnsafePathsForGCSubstitute := make([]*util.AccessPath, 0) // When NO_INDEX_LOOKUP_PUSHDOWN hint is specified, we should set `forceNoIndexLookUpPushDown = true` to avoid // using index look up push down even if other hint or system variable `tidb_index_lookup_pushdown_policy` @@ -1346,6 +1401,13 @@ func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, in continue } } + if index.ContainsVirtualGeneratedTemporalWithDateColumn(tblInfo) { + virtualGeneratedTemporalWithDateIndexes = append(virtualGeneratedTemporalWithDateIndexes, index) + if path := buildGcSubstitutePathForFilteredUnsafeIndex(tblInfo, index); path != nil { + filteredUnsafePathsForGCSubstitute = append(filteredUnsafePathsForGCSubstitute, path) + } + continue + } if index.InvertedInfo != nil { invertedIndexes[index.Name.L] = struct{}{} continue @@ -1396,6 +1458,7 @@ func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, in hasScanHint, hasUseOrForce := false, false available := make([]*util.AccessPath, 0, len(publicPaths)) ignored := make([]*util.AccessPath, 0, len(publicPaths)) + ignoredFilteredUnsafeIndexes := make(map[string]struct{}) // Extract comment-style index hint like /*+ INDEX(t, idx1, idx2) */. indexHintsLen := len(indexHints) @@ -1441,8 +1504,17 @@ func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, in if _, ok := invertedIndexes[idxName.L]; ok { continue } - path := getPathByIndexName(publicPaths, idxName, tblInfo) - if path == nil { + path, filteredIndex, resolveStatus := resolveIndexHintByName(publicPaths, virtualGeneratedTemporalWithDateIndexes, idxName, tblInfo) + if resolveStatus == indexHintResolveFilteredUnsafe { + if hint.HintType == ast.HintIgnore { + ignoredFilteredUnsafeIndexes[filteredIndex.Name.L] = struct{}{} + } else { + hasUseOrForce = true + ctx.GetSessionVars().StmtCtx.SetHintWarning(genVirtualGeneratedTemporalWithDateIndexHintWarning(filteredIndex)) + } + continue + } + if resolveStatus == indexHintResolveNotFound { err := plannererrors.ErrKeyDoesNotExist.FastGenByArgs(idxName, tblInfo.Name) // if hint is from comment-style sql hints, we should throw a warning instead of error. if i < indexHintsLen { @@ -1528,7 +1600,43 @@ func getPossibleAccessPaths(ctx base.PlanContext, tableHints *hint.PlanHints, in available = append(available, tablePath) } - return available, nil + return &possibleAccessPathInfo{ + paths: available, + gcSubstituteExtraPaths: getGCSubstituteExtraPaths(filteredUnsafePathsForGCSubstitute, hasUseOrForce, ignoredFilteredUnsafeIndexes), + }, nil +} + +func buildGcSubstitutePathForFilteredUnsafeIndex(tblInfo *model.TableInfo, index *model.IndexInfo) *util.AccessPath { + if index.InvertedInfo != nil { + return nil + } + if index.IsColumnarIndex() { + // Keep the original substitution behavior for columnar indexes only when the replica is available. + if tblInfo.TiFlashReplica == nil || !tblInfo.TiFlashReplica.Available { + return nil + } + path := genTiFlashPath(tblInfo) + path.Index = index + return path + } + return &util.AccessPath{Index: index} +} + +func getGCSubstituteExtraPaths(filteredUnsafePaths []*util.AccessPath, hasUseOrForce bool, ignoredFilteredUnsafeIndexes map[string]struct{}) []*util.AccessPath { + if hasUseOrForce || len(filteredUnsafePaths) == 0 { + return nil + } + extraPaths := make([]*util.AccessPath, 0, len(filteredUnsafePaths)) + for _, path := range filteredUnsafePaths { + if path.Index == nil { + continue + } + if _, ignored := ignoredFilteredUnsafeIndexes[path.Index.Name.L]; ignored { + continue + } + extraPaths = append(extraPaths, path) + } + return extraPaths } func removeIgnoredPaths(paths, ignoredPaths []*util.AccessPath) []*util.AccessPath { diff --git a/pkg/planner/core/planbuilder_test.go b/pkg/planner/core/planbuilder_test.go index 9063da8553720..93ca325d843d9 100644 --- a/pkg/planner/core/planbuilder_test.go +++ b/pkg/planner/core/planbuilder_test.go @@ -106,29 +106,93 @@ func TestGetPathByIndexName(t *testing.T) { genTiFlashPath(tblInfo), } - path := getPathByIndexName(accessPath, ast.NewCIStr("idx"), tblInfo) + resolveHint := func(paths []*util.AccessPath, filteredUnsafeIndexes []*model.IndexInfo, idxName string, tblInfo *model.TableInfo) (*util.AccessPath, *model.IndexInfo, indexHintResolveStatus) { + return resolveIndexHintByName(paths, filteredUnsafeIndexes, ast.NewCIStr(idxName), tblInfo) + } + + path, filteredIndex, status := resolveHint(accessPath, nil, "idx", tblInfo) require.NotNil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolvePublicPath, status) require.Equal(t, accessPath[1], path) // "id" is a prefix of "idx" - path = getPathByIndexName(accessPath, ast.NewCIStr("id"), tblInfo) + path, filteredIndex, status = resolveHint(accessPath, nil, "id", tblInfo) require.NotNil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolvePublicPath, status) require.Equal(t, accessPath[1], path) - path = getPathByIndexName(accessPath, ast.NewCIStr("primary"), tblInfo) + path, filteredIndex, status = resolveHint(accessPath, nil, "primary", tblInfo) require.NotNil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolvePublicPath, status) require.Equal(t, accessPath[0], path) - path = getPathByIndexName(accessPath, ast.NewCIStr("not exists"), tblInfo) + path, filteredIndex, status = resolveHint(accessPath, nil, "not exists", tblInfo) require.Nil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolveNotFound, status) tblInfo = &model.TableInfo{ Indices: make([]*model.IndexInfo, 0), PKIsHandle: false, } - path = getPathByIndexName(accessPath, ast.NewCIStr("primary"), tblInfo) + path, filteredIndex, status = resolveHint(accessPath, nil, "primary", tblInfo) require.Nil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolveNotFound, status) + + t.Run("resolve filtered unsafe indexes", func(t *testing.T) { + unsafeExact := &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe")} + unsafeLong := &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe_long")} + + path, filteredIndex, status = resolveHint(accessPath, []*model.IndexInfo{unsafeExact, unsafeLong}, "idx_unsafe", tblInfo) + require.Nil(t, path) + require.Same(t, unsafeExact, filteredIndex) + require.Equal(t, indexHintResolveFilteredUnsafe, status) + + path, filteredIndex, status = resolveHint(accessPath, []*model.IndexInfo{unsafeLong}, "idx_unsafe_l", tblInfo) + require.Nil(t, path) + require.Same(t, unsafeLong, filteredIndex) + require.Equal(t, indexHintResolveFilteredUnsafe, status) + + publicPath := &util.AccessPath{Index: &model.IndexInfo{Name: ast.NewCIStr("idx_shared")}} + filteredUnsafe := &model.IndexInfo{Name: ast.NewCIStr("idx_shared")} + path, filteredIndex, status = resolveHint([]*util.AccessPath{publicPath}, []*model.IndexInfo{filteredUnsafe}, "idx_shared", tblInfo) + require.Same(t, publicPath, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolvePublicPath, status) + + ambiguousShort := &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe_short")} + ambiguousLong := &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe_long")} + path, filteredIndex, status = resolveHint(accessPath, []*model.IndexInfo{ambiguousShort, ambiguousLong}, "idx_unsafe_", tblInfo) + require.Nil(t, path) + require.Nil(t, filteredIndex) + require.Equal(t, indexHintResolveNotFound, status) + }) + + t.Run("gc substitute extra paths", func(t *testing.T) { + filteredUnsafePaths := []*util.AccessPath{ + {Index: &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe")}}, + {Index: &model.IndexInfo{Name: ast.NewCIStr("idx_unsafe_two")}}, + } + + extraPaths := getGCSubstituteExtraPaths(filteredUnsafePaths, false, nil) + require.Len(t, extraPaths, 2) + require.Same(t, filteredUnsafePaths[0], extraPaths[0]) + require.Same(t, filteredUnsafePaths[1], extraPaths[1]) + + extraPaths = getGCSubstituteExtraPaths(filteredUnsafePaths, true, nil) + require.Nil(t, extraPaths) + + extraPaths = getGCSubstituteExtraPaths(filteredUnsafePaths, false, map[string]struct{}{ + "idx_unsafe": {}, + }) + require.Len(t, extraPaths, 1) + require.Same(t, filteredUnsafePaths[1], extraPaths[0]) + }) t.Run("ignore exact and prefix-resolved long index without removing shorter sibling", func(t *testing.T) { shortPath := &util.AccessPath{Index: &model.IndexInfo{Name: ast.NewCIStr("idx_contract_sys_no")}} @@ -139,19 +203,28 @@ func TestGetPathByIndexName(t *testing.T) { Indices: []*model.IndexInfo{shortPath.Index, longPath.Index}, } - ignored := []*util.AccessPath{getPathByIndexName(paths, ast.NewCIStr("idx_contract_sys_no_delete_flag"), tblInfo)} + ignoredPath, ignoredIndex, resolveStatus := resolveHint(paths, nil, "idx_contract_sys_no_delete_flag", tblInfo) + require.Nil(t, ignoredIndex) + require.Equal(t, indexHintResolvePublicPath, resolveStatus) + ignored := []*util.AccessPath{ignoredPath} require.Same(t, longPath, ignored[0]) remained := removeIgnoredPaths(paths, ignored) require.Len(t, remained, 1) require.Same(t, shortPath, remained[0]) - ignored = []*util.AccessPath{getPathByIndexName(paths, ast.NewCIStr("idx_contract_sys_no_delete"), tblInfo)} + ignoredPath, ignoredIndex, resolveStatus = resolveHint(paths, nil, "idx_contract_sys_no_delete", tblInfo) + require.Nil(t, ignoredIndex) + require.Equal(t, indexHintResolvePublicPath, resolveStatus) + ignored = []*util.AccessPath{ignoredPath} require.Same(t, longPath, ignored[0]) remained = removeIgnoredPaths(paths, ignored) require.Len(t, remained, 1) require.Same(t, shortPath, remained[0]) - ignored = []*util.AccessPath{getPathByIndexName(paths, ast.NewCIStr("Idx_Contract_Sys_No_Delete_Flag"), tblInfo)} + ignoredPath, ignoredIndex, resolveStatus = resolveHint(paths, nil, "Idx_Contract_Sys_No_Delete_Flag", tblInfo) + require.Nil(t, ignoredIndex) + require.Equal(t, indexHintResolvePublicPath, resolveStatus) + ignored = []*util.AccessPath{ignoredPath} require.Same(t, longPath, ignored[0]) remained = removeIgnoredPaths(paths, ignored) require.Len(t, remained, 1) diff --git a/pkg/planner/core/rule_generate_column_substitute.go b/pkg/planner/core/rule_generate_column_substitute.go index 7e369a3123b3e..8112ea739ede7 100644 --- a/pkg/planner/core/rule_generate_column_substitute.go +++ b/pkg/planner/core/rule_generate_column_substitute.go @@ -21,6 +21,7 @@ import ( "github.com/pingcap/tidb/pkg/parser/ast" "github.com/pingcap/tidb/pkg/planner/core/base" "github.com/pingcap/tidb/pkg/planner/core/operator/logicalop" + "github.com/pingcap/tidb/pkg/planner/util" "github.com/pingcap/tidb/pkg/types" h "github.com/pingcap/tidb/pkg/util/hint" ) @@ -70,25 +71,32 @@ func collectGenerateColumn(lp base.LogicalPlan, exprToColumn ExprColumnMap) { if ds.PreferStoreType&h.PreferTiFlash != 0 { return } - ectx := lp.SCtx().GetExprCtx().GetEvalCtx() for _, p := range ds.AllPossibleAccessPaths { - if p.IsTablePath() { - continue - } - for _, idxPart := range p.Index.Columns { - colInfo := ds.TableInfo.Columns[idxPart.Offset] - if colInfo.IsGenerated() && !colInfo.GeneratedStored { - s := ds.Schema().Columns - col := expression.ColInfo2Col(s, colInfo) - if col != nil && col.GetType(ectx).PartialEqual(col.VirtualExpr.GetType(ectx), lp.SCtx().GetSessionVars().EnableUnsafeSubstitute) { - // Replacing a literal with a generated column that is itself a pure constant is not - // semantically neutral across outer joins, because null-augmentation can turn the - // generated column into NULL while the original literal stays constant. - if len(expression.ExtractColumns(col.VirtualExpr)) == 0 { - continue - } - exprToColumn[col.VirtualExpr] = col + collectGeneratedColumnsFromPath(lp, ds, p, exprToColumn) + } + for _, p := range ds.GcSubstituteExtraPaths { + collectGeneratedColumnsFromPath(lp, ds, p, exprToColumn) + } +} + +func collectGeneratedColumnsFromPath(lp base.LogicalPlan, ds *logicalop.DataSource, path *util.AccessPath, exprToColumn ExprColumnMap) { + if path == nil || path.IsTablePath() || path.Index == nil { + return + } + ectx := lp.SCtx().GetExprCtx().GetEvalCtx() + for _, idxPart := range path.Index.Columns { + colInfo := ds.TableInfo.Columns[idxPart.Offset] + if colInfo.IsGenerated() && !colInfo.GeneratedStored { + s := ds.Schema().Columns + col := expression.ColInfo2Col(s, colInfo) + if col != nil && col.GetType(ectx).PartialEqual(col.VirtualExpr.GetType(ectx), lp.SCtx().GetSessionVars().EnableUnsafeSubstitute) { + // Replacing a literal with a generated column that is itself a pure constant is not + // semantically neutral across outer joins, because null-augmentation can turn the + // generated column into NULL while the original literal stays constant. + if len(expression.ExtractColumns(col.VirtualExpr)) == 0 { + continue } + exprToColumn[col.VirtualExpr] = col } } }