sql: change RequireActiveIndex to IncludeNonActiveIndex

The default value of a `bool` field should encode the common case. The common case for index resolution should be to include only active indexes. So this patch changes `RequireActiveIndex` (`false` is non-common case) to `IncludeNonActiveIndex` (`false` becomes common case). Release note: None
adityamaru · Dec 22, 2022 · dfdc8af · dfdc8af
1 parent 47ba970
commit dfdc8af
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 28 deletions.
diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go
@@ -523,7 +523,7 @@ func ResolveIndex(
 
 		idx, err := candidateTbl.FindNonDropIndexWithName(string(tableIndexName.Index))
 		// err == nil indicates that the index is found.
-		if err == nil && (!flag.RequireActiveIndex || idx.Public()) {
+		if err == nil && (flag.IncludeNonActiveIndex || idx.Public()) {
 			return true, resolvedPrefix, candidateTbl, idx, nil
 		}
 
@@ -548,7 +548,7 @@ func ResolveIndex(
 		}
 
 		tblFound, tbl, idx, err := findTableContainingIndex(
-			ctx, tree.Name(tableIndexName.Index), resolvedPrefix, schemaResolver, flag.RequireActiveIndex, flag.IncludeOfflineTable,
+			ctx, tree.Name(tableIndexName.Index), resolvedPrefix, schemaResolver, flag.IncludeNonActiveIndex, flag.IncludeOfflineTable,
 		)
 		if err != nil {
 			return false, catalog.ResolvedObjectPrefix{}, nil, nil, err
@@ -591,7 +591,7 @@ func ResolveIndex(
 		schemaFound = true
 
 		candidateFound, tbl, idx, curErr := findTableContainingIndex(
-			ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, schemaResolver, flag.RequireActiveIndex, flag.IncludeOfflineTable,
+			ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, schemaResolver, flag.IncludeNonActiveIndex, flag.IncludeOfflineTable,
 		)
 		if curErr != nil {
 			return false, catalog.ResolvedObjectPrefix{}, nil, nil, curErr
@@ -702,7 +702,7 @@ func findTableContainingIndex(
 	indexName tree.Name,
 	resolvedPrefix catalog.ResolvedObjectPrefix,
 	schemaResolver SchemaResolver,
-	requireActiveIndex bool,
+	includeNonActiveIndex bool,
 	includeOfflineTable bool,
 ) (found bool, tblDesc catalog.TableDescriptor, idxDesc catalog.Index, err error) {
 	dsNames, _, err := schemaResolver.GetObjectNamesAndIDs(ctx, resolvedPrefix.Database, resolvedPrefix.Schema)
@@ -732,7 +732,7 @@ func findTableContainingIndex(
 
 		candidateIdx, err := candidateTbl.FindNonDropIndexWithName(string(indexName))
 		if err == nil {
-			if requireActiveIndex && !candidateIdx.Public() {
+			if !includeNonActiveIndex && !candidateIdx.Public() {
 				continue
 			}
 			if found {

diff --git a/pkg/sql/catalog/resolver/resolver_test.go b/pkg/sql/catalog/resolver/resolver_test.go
@@ -637,7 +637,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`,
 		for _, tc := range testCases {
 			t.Run(tc.testName, func(t *testing.T) {
 				_, prefix, tblDesc, idxDesc, err := resolver.ResolveIndex(
-					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: true, RequireActiveIndex: false})
+					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: true, IncludeNonActiveIndex: true})
 				var res string
 				if err != nil {
 					res = fmt.Sprintf("error: %s", err.Error())
@@ -647,7 +647,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`,
 				require.Equal(t, tc.expected, res)
 
 				_, _, _, _, err = resolver.ResolveIndex(
-					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: false, RequireActiveIndex: false})
+					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: false, IncludeNonActiveIndex: true})
 				if tc.errIfNotRequired {
 					require.Error(t, err)
 				} else {
@@ -722,9 +722,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "baz_idx"),
 			tree.IndexLookupFlags{
-				Required:            false,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: false,
+				Required:              false,
+				IncludeNonActiveIndex: false,
+				IncludeOfflineTable:   false,
 			},
 		)
 		require.NoError(t, err)
@@ -737,9 +737,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "baz_idx"),
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  false,
-				IncludeOfflineTable: true,
+				Required:              true,
+				IncludeNonActiveIndex: true,
+				IncludeOfflineTable:   true,
 			},
 		)
 		require.NoError(t, err)
@@ -753,9 +753,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "foo_idx"),
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: false,
+				Required:              true,
+				IncludeNonActiveIndex: false,
+				IncludeOfflineTable:   false,
 			},
 		)
 		require.NoError(t, err)

diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go
@@ -263,9 +263,9 @@ func (oc *optCatalog) ResolveIndex(
 			oc.planner,
 			name,
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: flags.IncludeOfflineTables,
+				Required:              true,
+				IncludeNonActiveIndex: false,
+				IncludeOfflineTable:   flags.IncludeOfflineTables,
 			},
 		)
 	})

diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go
@@ -720,7 +720,8 @@ func expandIndexName(
 		p,
 		index,
 		tree.IndexLookupFlags{
-			Required: requireTable,
+			Required:              requireTable,
+			IncludeNonActiveIndex: true,
 		},
 	)
 	if err != nil {
@@ -768,8 +769,8 @@ func (p *planner) getTableAndIndexImpl(
 	_, resolvedPrefix, tbl, idx, err := resolver.ResolveIndex(
 		ctx, p, tableWithIndex,
 		tree.IndexLookupFlags{
-			Required:           true,
-			RequireActiveIndex: true,
+			Required:              true,
+			IncludeNonActiveIndex: false,
 		},
 	)
 	if err != nil {

diff --git a/pkg/sql/schemachanger/scdeps/build_deps.go b/pkg/sql/schemachanger/scdeps/build_deps.go
@@ -170,7 +170,7 @@ func (d *buildDeps) MayResolveIndex(
 	idx catalog.Index,
 ) {
 	found, prefix, tbl, idx, err := resolver.ResolveIndex(
-		ctx, d.schemaResolver, &tableIndexName, tree.IndexLookupFlags{},
+		ctx, d.schemaResolver, &tableIndexName, tree.IndexLookupFlags{IncludeNonActiveIndex: true},
 	)
 
 	if err != nil {

diff --git a/pkg/sql/sem/tree/name_resolution.go b/pkg/sql/sem/tree/name_resolution.go
@@ -273,11 +273,14 @@ func ObjectLookupFlagsWithRequiredTableKind(kind RequiredTableKind) ObjectLookup
 
 // IndexLookupFlags is the flag struct used for resolver.ResolveIndex() only.
 type IndexLookupFlags struct {
-	// Control if the lookup can return nil index without returning an error if
-	// the index does not exist.
+	// Required, if true, indicates lookup can return nil index without
+	// returning an error if the index does not exist.
 	Required bool
-	// Control if the lookup only considers active indexes.
-	RequireActiveIndex bool
-	// Control if the lookup considers offline tables.
+	// IncludeNonActiveIndex expands the lookup to also consider
+	// non-active indexes. By default, only active indexes are
+	// considered.
+	IncludeNonActiveIndex bool
+	// IncludeOfflineTable expands the lookup to also consider offline
+	// tables. By default, only online tables are considered.
 	IncludeOfflineTable bool
 }