sql: add ALTER INDEX … NOT VISIBLE to parser

This commit adds parsing support for ALTER INDEX … [VISIBLE | NOT VISIBLE]. Executing the command returns an `unimplemented error`. Assists: cockroachdb#72576 See also: cockroachdb#84783 Release note (sql change): Parser now supports altering an index to visible or not visible. But no implementation has done yet, and executing it returns an “unimplemented” error immediately. # Conflicts: # pkg/sql/sem/tree/stmt.go
wenyihu6 · Aug 11, 2022 · e624798 · e624798
1 parent ef4970f
commit e624798
Show file tree

Hide file tree

Showing 18 changed files with 200 additions and 5 deletions.
diff --git a/docs/generated/sql/bnf/BUILD.bazel b/docs/generated/sql/bnf/BUILD.bazel
@@ -30,6 +30,7 @@ FILES = [
     "alter_func_dep_extension_stmt",
     "alter_index_partition_by",
     "alter_index_stmt",
+    "alter_index_visible_stmt",
     "alter_partition_stmt",
     "alter_primary_key",
     "alter_range_relocate_stmt",

diff --git a/docs/generated/sql/bnf/alter_index_stmt.bnf b/docs/generated/sql/bnf/alter_index_stmt.bnf
@@ -5,3 +5,4 @@ alter_index_stmt ::=
 	| alter_scatter_index_stmt
 	| alter_rename_index_stmt
 	| alter_zone_index_stmt
+	| alter_index_visible_stmt
diff --git a/docs/generated/sql/bnf/alter_index_visible_stmt.bnf b/docs/generated/sql/bnf/alter_index_visible_stmt.bnf
@@ -0,0 +1,3 @@
+alter_index_visible_stmt ::=
+	'ALTER' 'INDEX' table_index_name alter_index_visible
+	| 'ALTER' 'INDEX' 'IF' 'EXISTS' table_index_name alter_index_visible
diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -1503,6 +1503,7 @@ alter_index_stmt ::=
 	| alter_scatter_index_stmt
 	| alter_rename_index_stmt
 	| alter_zone_index_stmt
+	| alter_index_visible_stmt
 
 alter_view_stmt ::=
 	alter_rename_view_stmt
@@ -2034,6 +2035,10 @@ alter_rename_index_stmt ::=
 alter_zone_index_stmt ::=
 	'ALTER' 'INDEX' table_index_name set_zone_config
 
+alter_index_visible_stmt ::=
+	'ALTER' 'INDEX' table_index_name alter_index_visible
+	| 'ALTER' 'INDEX' 'IF' 'EXISTS' table_index_name alter_index_visible
+
 alter_rename_view_stmt ::=
 	'ALTER' 'VIEW' relation_expr 'RENAME' 'TO' view_name
 	| 'ALTER' 'MATERIALIZED' 'VIEW' relation_expr 'RENAME' 'TO' view_name
@@ -2694,6 +2699,10 @@ locality ::=
 alter_index_cmds ::=
 	( alter_index_cmd ) ( ( ',' alter_index_cmd ) )*
 
+alter_index_visible ::=
+	'NOT' 'VISIBLE'
+	| 'VISIBLE'
+
 sequence_option_list ::=
 	( sequence_option_elem ) ( ( sequence_option_elem ) )*
 

diff --git a/pkg/gen/bnf.bzl b/pkg/gen/bnf.bzl
@@ -30,6 +30,7 @@ BNF_SRCS = [
   "//docs/generated/sql/bnf:alter_func_stmt.bnf",
   "//docs/generated/sql/bnf:alter_index_partition_by.bnf",
   "//docs/generated/sql/bnf:alter_index_stmt.bnf",
+  "//docs/generated/sql/bnf:alter_index_visible_stmt.bnf",
   "//docs/generated/sql/bnf:alter_partition_stmt.bnf",
   "//docs/generated/sql/bnf:alter_primary_key.bnf",
   "//docs/generated/sql/bnf:alter_range_relocate_stmt.bnf",

diff --git a/pkg/gen/diagrams.bzl b/pkg/gen/diagrams.bzl
@@ -31,6 +31,7 @@ DIAGRAMS_SRCS = [
   "//docs/generated/sql/bnf:alter_func_set_schema.html",
   "//docs/generated/sql/bnf:alter_index.html",
   "//docs/generated/sql/bnf:alter_index_partition_by.html",
+  "//docs/generated/sql/bnf:alter_index_visible.html",
   "//docs/generated/sql/bnf:alter_partition.html",
   "//docs/generated/sql/bnf:alter_primary_key.html",
   "//docs/generated/sql/bnf:alter_range.html",

diff --git a/pkg/gen/docs.bzl b/pkg/gen/docs.bzl
@@ -42,6 +42,7 @@ DOCS_SRCS = [
   "//docs/generated/sql/bnf:alter_func_stmt.bnf",
   "//docs/generated/sql/bnf:alter_index_partition_by.bnf",
   "//docs/generated/sql/bnf:alter_index_stmt.bnf",
+  "//docs/generated/sql/bnf:alter_index_visible_stmt.bnf",
   "//docs/generated/sql/bnf:alter_partition_stmt.bnf",
   "//docs/generated/sql/bnf:alter_primary_key.bnf",
   "//docs/generated/sql/bnf:alter_range_relocate_stmt.bnf",

diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel
@@ -13,6 +13,7 @@ go_library(
         "alter_default_privileges.go",
         "alter_function.go",
         "alter_index.go",
+        "alter_index_visible.go",
         "alter_primary_key.go",
         "alter_role.go",
         "alter_schema.go",

diff --git a/pkg/sql/alter_index_visible.go b/pkg/sql/alter_index_visible.go
@@ -0,0 +1,37 @@
+package sql
+
+import (
+	"context"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+type alterIndexVisibleNode struct {
+	n         *tree.AlterIndexVisible
+	tableDesc *tabledesc.Mutable
+	index     catalog.Index
+}
+
+func (p *planner) AlterIndexVisible(ctx context.Context, stmt *tree.AlterIndexVisible) (planNode, error) {
+	return nil, unimplemented.Newf(
+		"Not Visible Index",
+		"altering an index to visible or not visible is not supported yet")
+}
+
+// TODO (wenyihu6): Double check on whether we should have ReadingOwnWrites. I
+// don't think changing visibility performs multiple KV operations on descriptor
+// and expects to see its own writes. But I'm not certain since renameIndexNode
+// is also implementing this interface method.
+// func (n *alterIndexVisibleNode) ReadingOwnWrites() {}
+
+func (n *alterIndexVisibleNode) startExec(params runParams) error {
+	return unimplemented.Newf(
+		"Not Visible Index",
+		"altering an index to visible or not visible is not supported yet")
+}
+func (n *alterIndexVisibleNode) Next(runParams) (bool, error) { return false, nil }
+func (n *alterIndexVisibleNode) Values() tree.Datums          { return tree.Datums{} }
+func (n *alterIndexVisibleNode) Close(context.Context)        {}
diff --git a/pkg/sql/opaque.go b/pkg/sql/opaque.go
@@ -120,6 +120,8 @@ func planOpaque(ctx context.Context, p *planner, stmt tree.Statement) (planNode,
 		return p.AlterFunctionDepExtension(ctx, n)
 	case *tree.AlterIndex:
 		return p.AlterIndex(ctx, n)
+	case *tree.AlterIndexVisible:
+		return p.AlterIndexVisible(ctx, n)
 	case *tree.AlterSchema:
 		return p.AlterSchema(ctx, n)
 	case *tree.AlterTable:
@@ -296,6 +298,7 @@ func init() {
 		&tree.AlterFunctionSetSchema{},
 		&tree.AlterFunctionDepExtension{},
 		&tree.AlterIndex{},
+		&tree.AlterIndexVisible{},
 		&tree.AlterSchema{},
 		&tree.AlterTable{},
 		&tree.AlterTableLocality{},

diff --git a/pkg/sql/opt/exec/execbuilder/testdata/not_visible_index b/pkg/sql/opt/exec/execbuilder/testdata/not_visible_index
@@ -1153,3 +1153,24 @@ DROP TABLE child_update
 
 statement ok
 DROP TABLE parent
+
+############################################################################
+# The following tests check for ALTER INDEX ... VISIBLE | NOT VISIBLE. 
+############################################################################
+statement ok
+CREATE TABLE t (p INT PRIMARY KEY, INDEX idx (p) VISIBLE)
+
+query TTB
+SELECT index_name, column_name, visible FROM [SHOW INDEX FROM t] ORDER BY index_name, seq_in_index
+----
+idx     p  true
+t_pkey  p  true
+
+statement error pq: unimplemented: altering an index to visible or not visible is not supported yet
+ALTER INDEX idx NOT VISIBLE
+
+statement error pq: unimplemented: altering an index to visible or not visible is not supported yet
+ALTER INDEX idx VISIBLE
+
+statement ok
+DROP TABLE t
diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y
@@ -1040,6 +1040,7 @@ func (u *sqlSymUnion) functionObjs() tree.FuncObjs {
 %type <tree.Statement> alter_rename_index_stmt
 %type <tree.Statement> alter_relocate_index_stmt
 %type <tree.Statement> alter_zone_index_stmt
+%type <tree.Statement> alter_index_visible_stmt
 
 // ALTER VIEW
 %type <tree.Statement> alter_rename_view_stmt
@@ -1378,7 +1379,7 @@ func (u *sqlSymUnion) functionObjs() tree.FuncObjs {
 %type <tree.Expr> overlay_placing
 
 %type <bool> opt_unique opt_concurrently opt_cluster opt_without_index
-%type <bool> opt_index_access_method opt_index_visible
+%type <bool> opt_index_access_method opt_index_visible alter_index_visible
 
 %type <*tree.Limit> limit_clause offset_clause opt_limit_clause
 %type <tree.Expr> select_fetch_first_value
@@ -2088,6 +2089,7 @@ alter_range_stmt:
 //   ALTER INDEX ... UNSPLIT ALL
 //   ALTER INDEX ... SCATTER [ FROM ( <exprs...> ) TO ( <exprs...> ) ]
 //   ALTER INDEX ... RELOCATE [ LEASE | VOTERS | NONVOTERS ] <selectclause>
+//   ALTER INDEX ... [VISIBLE | NOT VISIBLE]
 //
 // Zone configurations:
 //   DISCARD
@@ -2104,6 +2106,7 @@ alter_index_stmt:
 | alter_scatter_index_stmt
 | alter_rename_index_stmt
 | alter_zone_index_stmt
+| alter_index_visible_stmt
 // ALTER INDEX has its error help token here because the ALTER INDEX
 // prefix is spread over multiple non-terminals.
 | ALTER INDEX error // SHOW HELP: ALTER INDEX
@@ -2236,6 +2239,26 @@ alter_relocate_index_stmt:
     }
   }
 
+alter_index_visible_stmt:
+  ALTER INDEX table_index_name alter_index_visible
+  {
+    $$.val = &tree.AlterIndexVisible{Index: $3.tableIndexName(), NotVisible: $4.bool(), IfExists: false}
+  }
+| ALTER INDEX IF EXISTS table_index_name alter_index_visible
+  {
+    $$.val = &tree.AlterIndexVisible{Index: $5.tableIndexName(), NotVisible: $6.bool(), IfExists: true}
+  }
+
+alter_index_visible:
+  NOT VISIBLE
+  {
+    $$.val = true
+  }
+| VISIBLE
+  {
+    $$.val = false
+  }
+
 // Note: even though the ALTER RANGE ... CONFIGURE ZONE syntax only
 // accepts unrestricted names in the 3rd position, such that we could
 // write:
@@ -9479,9 +9502,9 @@ opt_index_visible:
     $$.val = true
   }
 | VISIBLE
-   {
-     $$.val = false
-   }
+  {
+    $$.val = false
+  }
 | /* EMPTY */
   {
     $$.val = false

diff --git a/pkg/sql/parser/testdata/alter_index b/pkg/sql/parser/testdata/alter_index
@@ -359,3 +359,59 @@ ALTER INDEX i CONFIGURE ZONE USING DEFAULT
 ALTER INDEX i CONFIGURE ZONE USING DEFAULT -- fully parenthesized
 ALTER INDEX i CONFIGURE ZONE USING DEFAULT -- literals removed
 ALTER INDEX _ CONFIGURE ZONE USING DEFAULT -- identifiers removed
+
+parse
+ALTER INDEX i VISIBLE
+----
+ALTER INDEX i VISIBLE
+ALTER INDEX i VISIBLE -- fully parenthesized
+ALTER INDEX i VISIBLE -- literals removed
+ALTER INDEX _ VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX i NOT VISIBLE
+----
+ALTER INDEX i NOT VISIBLE
+ALTER INDEX i NOT VISIBLE -- fully parenthesized
+ALTER INDEX i NOT VISIBLE -- literals removed
+ALTER INDEX _ NOT VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX IF EXISTS i VISIBLE
+----
+ALTER INDEX IF EXISTS i VISIBLE
+ALTER INDEX IF EXISTS i VISIBLE -- fully parenthesized
+ALTER INDEX IF EXISTS i VISIBLE -- literals removed
+ALTER INDEX IF EXISTS _ VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX IF EXISTS i NOT VISIBLE
+----
+ALTER INDEX IF EXISTS i NOT VISIBLE
+ALTER INDEX IF EXISTS i NOT VISIBLE -- fully parenthesized
+ALTER INDEX IF EXISTS i NOT VISIBLE -- literals removed
+ALTER INDEX IF EXISTS _ NOT VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX d.i NOT VISIBLE
+----
+ALTER INDEX d.i NOT VISIBLE
+ALTER INDEX d.i NOT VISIBLE -- fully parenthesized
+ALTER INDEX d.i NOT VISIBLE -- literals removed
+ALTER INDEX _._ NOT VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX t@i NOT VISIBLE
+----
+ALTER INDEX t@i NOT VISIBLE
+ALTER INDEX t@i NOT VISIBLE -- fully parenthesized
+ALTER INDEX t@i NOT VISIBLE -- literals removed
+ALTER INDEX _@_ NOT VISIBLE -- identifiers removed
+
+parse
+ALTER INDEX db.t@i NOT VISIBLE
+----
+ALTER INDEX db.t@i NOT VISIBLE
+ALTER INDEX db.t@i NOT VISIBLE -- fully parenthesized
+ALTER INDEX db.t@i NOT VISIBLE -- literals removed
+ALTER INDEX _._@_ NOT VISIBLE -- identifiers removed
diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go
@@ -147,6 +147,7 @@ type planNodeReadingOwnWrites interface {
 }
 
 var _ planNode = &alterIndexNode{}
+var _ planNode = &alterIndexVisibleNode{}
 var _ planNode = &alterSchemaNode{}
 var _ planNode = &alterSequenceNode{}
 var _ planNode = &alterTableNode{}

diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go
@@ -58,7 +58,7 @@ func (p *planner) prepareUsingOptimizer(ctx context.Context) (planFlags, error)
 	opc.reset(ctx)
 
 	switch t := stmt.AST.(type) {
-	case *tree.AlterIndex, *tree.AlterTable, *tree.AlterSequence,
+	case *tree.AlterIndex, *tree.AlterIndexVisible, *tree.AlterTable, *tree.AlterSequence,
 		*tree.Analyze,
 		*tree.BeginTransaction,
 		*tree.CommentOnColumn, *tree.CommentOnConstraint, *tree.CommentOnDatabase, *tree.CommentOnIndex, *tree.CommentOnTable, *tree.CommentOnSchema,

diff --git a/pkg/sql/sem/tree/alter_index.go b/pkg/sql/sem/tree/alter_index.go
@@ -64,3 +64,26 @@ type AlterIndexPartitionBy struct {
 func (node *AlterIndexPartitionBy) Format(ctx *FmtCtx) {
 	ctx.FormatNode(node.PartitionByIndex)
 }
+
+// AlterIndexVisible represents a ALTER INDEX ... [VISIBLE | NOT VISIBLE] statement.
+type AlterIndexVisible struct {
+	Index      TableIndexName
+	NotVisible bool
+	IfExists   bool
+}
+
+var _ Statement = &AlterIndexVisible{}
+
+// Format implements the NodeFormatter interface.
+func (node *AlterIndexVisible) Format(ctx *FmtCtx) {
+	ctx.WriteString("ALTER INDEX ")
+	if node.IfExists {
+		ctx.WriteString("IF EXISTS ")
+	}
+	ctx.FormatNode(&node.Index)
+	if node.NotVisible {
+		ctx.WriteString(" NOT VISIBLE")
+	} else {
+		ctx.WriteString(" VISIBLE")
+	}
+}
diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go
@@ -363,6 +363,17 @@ func (*AlterIndex) StatementTag() string { return "ALTER INDEX" }
 
 func (*AlterIndex) hiddenFromShowQueries() {}
 
+// StatementReturnType implements the Statement interface.
+func (*AlterIndexVisible) StatementReturnType() StatementReturnType { return DDL }
+
+// StatementType implements the Statement interface.
+func (*AlterIndexVisible) StatementType() StatementType { return TypeDDL }
+
+// StatementTag returns a short string identifying the type of statement.
+func (*AlterIndexVisible) StatementTag() string { return "ALTER INDEX" }
+
+func (*AlterIndexVisible) hiddenFromShowQueries() {}
+
 // StatementReturnType implements the Statement interface.
 func (*AlterTable) StatementReturnType() StatementReturnType { return DDL }
 
@@ -1924,6 +1935,7 @@ func (n *AlterChangefeed) String() string                     { return AsString(
 func (n *AlterChangefeedCmds) String() string                 { return AsString(n) }
 func (n *AlterBackup) String() string                         { return AsString(n) }
 func (n *AlterIndex) String() string                          { return AsString(n) }
+func (n *AlterIndexVisible) String() string                   { return AsString(n) }
 func (n *AlterDatabaseOwner) String() string                  { return AsString(n) }
 func (n *AlterDatabaseAddRegion) String() string              { return AsString(n) }
 func (n *AlterDatabaseDropRegion) String() string             { return AsString(n) }

diff --git a/pkg/sql/walk.go b/pkg/sql/walk.go
@@ -353,6 +353,7 @@ var planNodeNames = map[reflect.Type]string{
 	reflect.TypeOf(&alterFunctionSetSchemaNode{}):              "alter function set schema",
 	reflect.TypeOf(&alterFunctionDepExtensionNode{}):           "alter function depends on extension",
 	reflect.TypeOf(&alterIndexNode{}):                          "alter index",
+	reflect.TypeOf(&alterIndexVisibleNode{}):                   "alter index visibility",
 	reflect.TypeOf(&alterSequenceNode{}):                       "alter sequence",
 	reflect.TypeOf(&alterSchemaNode{}):                         "alter schema",
 	reflect.TypeOf(&alterTableNode{}):                          "alter table",