Skip to content

Commit

Permalink
sql: use a 3-valued type instead of 2 booleans for relocate statements
Browse files Browse the repository at this point in the history
By using a 3-valued types, we greatly simplify the code to print out
statement types, and also clarify intent in all the conditionals that
depend on the relocation mode.

Release note: None
  • Loading branch information
knz committed Dec 14, 2021
1 parent 393268c commit 4092554
Show file tree
Hide file tree
Showing 22 changed files with 250 additions and 271 deletions.
6 changes: 2 additions & 4 deletions docs/generated/sql/bnf/alter_range_relocate_stmt.bnf
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
alter_range_relocate_stmt ::=
'ALTER' 'RANGE' relocate_kw 'LEASE' 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw 'LEASE' 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw 'NONVOTERS' 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw 'NONVOTERS' 'FROM' iconst64 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64
9 changes: 4 additions & 5 deletions docs/generated/sql/bnf/stmt_block.bnf
Original file line number Diff line number Diff line change
Expand Up @@ -1848,10 +1848,8 @@ alter_zone_range_stmt ::=
alter_range_relocate_stmt ::=
'ALTER' 'RANGE' relocate_kw 'LEASE' 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw 'LEASE' 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw 'NONVOTERS' 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw 'NONVOTERS' 'FROM' iconst64 'TO' iconst64
| 'ALTER' 'RANGE' relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
| 'ALTER' 'RANGE' iconst64 relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64

alter_zone_partition_stmt ::=
'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name set_zone_config
Expand Down Expand Up @@ -2333,9 +2331,10 @@ relocate_kw ::=
| 'EXPERIMENTAL_RELOCATE'
| 'RELOCATE'

voters_kw ::=
relocate_subject_nonlease ::=
'VOTERS'
|
| 'NONVOTERS'

alter_default_privileges_target_object ::=
'TABLES'
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/distsql_spec_exec_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -1070,13 +1070,13 @@ func (e *distSQLSpecExecFactory) ConstructAlterTableUnsplitAll(index cat.Index)
}

func (e *distSQLSpecExecFactory) ConstructAlterTableRelocate(
index cat.Index, input exec.Node, relocateLease bool, relocateNonVoters bool,
index cat.Index, input exec.Node, relocateSubject tree.RelocateSubject,
) (exec.Node, error) {
return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: alter table relocate")
}

func (e *distSQLSpecExecFactory) ConstructAlterRangeRelocate(
input exec.Node, relocateLease bool, relocateNonVoters bool, toStoreID int64, fromStoreID int64,
input exec.Node, relocateSubject tree.RelocateSubject, toStoreID int64, fromStoreID int64,
) (exec.Node, error) {
return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: alter range relocate")
}
Expand Down
6 changes: 2 additions & 4 deletions pkg/sql/opt/exec/execbuilder/statement.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,7 @@ func (b *Builder) buildAlterTableRelocate(relocate *memo.AlterTableRelocateExpr)
node, err := b.factory.ConstructAlterTableRelocate(
table.Index(relocate.Index),
input.root,
relocate.RelocateLease,
relocate.RelocateNonVoters,
relocate.SubjectReplicas,
)
if err != nil {
return execPlan{}, err
Expand All @@ -238,8 +237,7 @@ func (b *Builder) buildAlterRangeRelocate(relocate *memo.AlterRangeRelocateExpr)
}
node, err := b.factory.ConstructAlterRangeRelocate(
input.root,
relocate.RelocateLease,
relocate.RelocateNonVoters,
relocate.SubjectReplicas,
relocate.ToStoreID,
relocate.FromStoreID,
)
Expand Down
6 changes: 2 additions & 4 deletions pkg/sql/opt/exec/factory.opt
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,7 @@ define AlterTableUnsplitAll {
define AlterTableRelocate {
Index cat.Index
input exec.Node
relocateLease bool
relocateNonVoters bool
subjectReplicas tree.RelocateSubject
}

# Buffer passes through the input rows but also saves them in a buffer, which
Expand Down Expand Up @@ -736,8 +735,7 @@ define Export {
# AlterTableRelocate implements ALTER RANGE RELOCATE.
define AlterRangeRelocate {
input exec.Node
relocateLease bool
relocateNonVoters bool
subjectReplicas tree.RelocateSubject
toStoreID int64
fromStoreID int64
}
7 changes: 4 additions & 3 deletions pkg/sql/opt/memo/expr_format.go
Original file line number Diff line number Diff line change
Expand Up @@ -1502,11 +1502,12 @@ func FormatPrivate(f *ExprFmtCtx, private interface{}, physProps *physical.Requi

case *AlterTableRelocatePrivate:
FormatPrivate(f, &t.AlterTableSplitPrivate, nil)
if t.RelocateLease {
switch t.SubjectReplicas {
case tree.RelocateLease:
f.Buffer.WriteString(" [lease]")
} else if t.RelocateNonVoters {
case tree.RelocateNonVoters:
f.Buffer.WriteString(" [non-voters]")
} else {
case tree.RelocateVoters:
f.Buffer.WriteString(" [voters]")
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/sql/opt/memo/interner.go
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,10 @@ func (h *hasher) HashIndexOrdinal(val cat.IndexOrdinal) {
h.HashInt(val)
}

func (h *hasher) HashRelocateSubject(val tree.RelocateSubject) {
h.HashUint64(uint64(val))
}

func (h *hasher) HashIndexOrdinals(val cat.IndexOrdinals) {
hash := h.hash
for _, ord := range val {
Expand Down Expand Up @@ -978,6 +982,10 @@ func (h *hasher) IsIndexOrdinalEqual(l, r cat.IndexOrdinal) bool {
return l == r
}

func (h *hasher) IsRelocateSubjectEqual(l, r tree.RelocateSubject) bool {
return l == r
}

func (h *hasher) IsIndexOrdinalsEqual(l, r cat.IndexOrdinals) bool {
if len(l) != len(r) {
return false
Expand Down
6 changes: 2 additions & 4 deletions pkg/sql/opt/ops/statement.opt
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,7 @@ define AlterTableRelocate {

[Private]
define AlterTableRelocatePrivate {
RelocateLease bool
RelocateNonVoters bool
SubjectReplicas RelocateSubject
_ AlterTableSplitPrivate
}

Expand Down Expand Up @@ -303,8 +302,7 @@ define AlterRangeRelocate {

[Private]
define AlterRangeRelocatePrivate {
RelocateLease bool
RelocateNonVoters bool
SubjectReplicas RelocateSubject
ToStoreID int64
FromStoreID int64

Expand Down
16 changes: 6 additions & 10 deletions pkg/sql/opt/optbuilder/alter_range.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ func (b *Builder) buildAlterRangeRelocate(
outScope = inScope.push()
b.synthesizeResultColumns(outScope, colinfo.AlterTableRelocateColumns)

cmdName := "RELOCATE"
if relocate.RelocateLease {
cmdName += " LEASE"
}
cmdName := "RELOCATE " + relocate.SubjectReplicas.String()
colNames := []string{"range ids"}
colTypes := []*types.T{types.Int}

Expand All @@ -51,12 +48,11 @@ func (b *Builder) buildAlterRangeRelocate(
outScope.expr = b.factory.ConstructAlterRangeRelocate(
inputScope.expr,
&memo.AlterRangeRelocatePrivate{
RelocateLease: relocate.RelocateLease,
RelocateNonVoters: relocate.RelocateNonVoters,
ToStoreID: relocate.ToStoreID,
FromStoreID: relocate.FromStoreID,
Columns: colsToColList(outScope.cols),
Props: physical.MinRequired,
SubjectReplicas: relocate.SubjectReplicas,
ToStoreID: relocate.ToStoreID,
FromStoreID: relocate.FromStoreID,
Columns: colsToColList(outScope.cols),
Props: physical.MinRequired,
},
)
return outScope
Expand Down
7 changes: 3 additions & 4 deletions pkg/sql/opt/optbuilder/alter_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ func (b *Builder) buildAlterTableRelocate(

// The first column is the target leaseholder or the relocation array,
// depending on variant.
cmdName := "EXPERIMENTAL_RELOCATE"
if relocate.RelocateLease {
cmdName := "RELOCATE"
if relocate.SubjectReplicas == tree.RelocateLease {
cmdName += " LEASE"
colNames = append([]string{"target leaseholder"}, colNames...)
colTypes = append([]*types.T{types.Int}, colTypes...)
Expand All @@ -181,8 +181,7 @@ func (b *Builder) buildAlterTableRelocate(
outScope.expr = b.factory.ConstructAlterTableRelocate(
inputScope.expr,
&memo.AlterTableRelocatePrivate{
RelocateLease: relocate.RelocateLease,
RelocateNonVoters: relocate.RelocateNonVoters,
SubjectReplicas: relocate.SubjectReplicas,
AlterTableSplitPrivate: memo.AlterTableSplitPrivate{
Table: b.factory.Metadata().AddTable(table, &tn),
Index: index.Ordinal(),
Expand Down
14 changes: 7 additions & 7 deletions pkg/sql/opt/optbuilder/testdata/alter_range
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ CREATE TABLE abc (a INT PRIMARY KEY, b INT, c STRING, INDEX b (b), UNIQUE INDEX
build
ALTER RANGE 1 RELOCATE FROM 1 TO 2
----
alter-range-relocate &{false false 2 1 [3 4 5] []}
alter-range-relocate &{VOTERS 2 1 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── values
├── columns: column1:6!null
Expand All @@ -15,7 +15,7 @@ alter-range-relocate &{false false 2 1 [3 4 5] []}
build
ALTER RANGE RELOCATE FROM 1 TO 2 FOR SELECT a from abc
----
alter-range-relocate &{false false 2 1 [3 4 5] []}
alter-range-relocate &{VOTERS 2 1 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── project
├── columns: a:6!null
Expand All @@ -30,12 +30,12 @@ error (42601): at or near "relocate": syntax error
build
ALTER RANGE RELOCATE FROM 1 TO 2 FOR SELECT c from abc
----
error (42601): RELOCATE data column 1 (range ids) must be of type int, not type string
error (42601): RELOCATE VOTERS data column 1 (range ids) must be of type int, not type string

build
ALTER RANGE 1 RELOCATE NONVOTERS FROM 1 TO 2
----
alter-range-relocate &{false true 2 1 [3 4 5] []}
alter-range-relocate &{NONVOTERS 2 1 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── values
├── columns: column1:6!null
Expand All @@ -44,7 +44,7 @@ alter-range-relocate &{false true 2 1 [3 4 5] []}
build
ALTER RANGE RELOCATE NONVOTERS FROM 1 TO 2 FOR SELECT a from abc
----
alter-range-relocate &{false true 2 1 [3 4 5] []}
alter-range-relocate &{NONVOTERS 2 1 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── project
├── columns: a:6!null
Expand All @@ -55,7 +55,7 @@ alter-range-relocate &{false true 2 1 [3 4 5] []}
build
ALTER RANGE 1 RELOCATE LEASE TO 2
----
alter-range-relocate &{true false 2 0 [3 4 5] []}
alter-range-relocate &{LEASE 2 0 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── values
├── columns: column1:6!null
Expand All @@ -64,7 +64,7 @@ alter-range-relocate &{true false 2 0 [3 4 5] []}
build
ALTER RANGE RELOCATE LEASE TO 2 FOR SELECT a from abc
----
alter-range-relocate &{true false 2 0 [3 4 5] []}
alter-range-relocate &{LEASE 2 0 [3 4 5] []}
├── columns: range_id:3 pretty:4 result:5
└── project
├── columns: a:6!null
Expand Down
12 changes: 6 additions & 6 deletions pkg/sql/opt/optbuilder/testdata/alter_table
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,12 @@ alter-table-relocate abc [non-voters]
build
ALTER TABLE abc EXPERIMENTAL_RELOCATE LEASE VALUES (10), (11)
----
error (42601): less than 2 columns in EXPERIMENTAL_RELOCATE LEASE data
error (42601): less than 2 columns in RELOCATE LEASE data

build
ALTER TABLE abc EXPERIMENTAL_RELOCATE LEASE VALUES (10, 1, 2), (11, 3, 4)
----
error (42601): too many columns in EXPERIMENTAL_RELOCATE LEASE data
error (42601): too many columns in RELOCATE LEASE data

build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
Expand All @@ -179,22 +179,22 @@ alter-table-relocate abc@bc [non-voters]
build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (5, 1, 'foo'), (6, 2, 'bar')
----
error (42601): EXPERIMENTAL_RELOCATE data column 1 (relocation array) must be of type int[], not type int
error (42601): RELOCATE data column 1 (relocation array) must be of type int[], not type int

build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE LEASE VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
----
error (42601): EXPERIMENTAL_RELOCATE LEASE data column 1 (target leaseholder) must be of type int, not type int[]
error (42601): RELOCATE LEASE data column 1 (target leaseholder) must be of type int, not type int[]

build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (1, 2), (3, 4)
----
error (42601): EXPERIMENTAL_RELOCATE data column 1 (relocation array) must be of type int[], not type int
error (42601): RELOCATE data column 1 (relocation array) must be of type int[], not type int

build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (ARRAY[1,2], 1, 2), (ARRAY[3,4], 3, 4)
----
error (42601): EXPERIMENTAL_RELOCATE data column 3 (c) must be of type string, not type int
error (42601): RELOCATE data column 3 (c) must be of type string, not type int

build
ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE LEASE VALUES (10, 1, 'foo'), (11, 3, 'bar')
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/optgen/cmd/optgen/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ func newMetadata(compiled *lang.CompiledExpr, pkg string) *metadata {
"ScheduleCommand": {fullName: "tree.ScheduleCommand", passByVal: true},
"IndexOrdinal": {fullName: "cat.IndexOrdinal", passByVal: true},
"IndexOrdinals": {fullName: "cat.IndexOrdinals", passByVal: true},
"RelocateSubject": {fullName: "tree.RelocateSubject", passByVal: true},
"UniqueOrdinals": {fullName: "cat.UniqueOrdinals", passByVal: true},
"ViewDeps": {fullName: "opt.ViewDeps", passByVal: true},
"ViewTypeDeps": {fullName: "opt.ViewTypeDeps", passByVal: true},
Expand Down
22 changes: 10 additions & 12 deletions pkg/sql/opt_exec_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -1955,35 +1955,33 @@ func (ef *execFactory) ConstructAlterTableUnsplitAll(index cat.Index) (exec.Node

// ConstructAlterTableRelocate is part of the exec.Factory interface.
func (ef *execFactory) ConstructAlterTableRelocate(
index cat.Index, input exec.Node, relocateLease bool, relocateNonVoters bool,
index cat.Index, input exec.Node, relocateSubject tree.RelocateSubject,
) (exec.Node, error) {
if !ef.planner.ExecCfg().Codec.ForSystemTenant() {
return nil, errorutil.UnsupportedWithMultiTenancy(54250)
}

return &relocateNode{
relocateLease: relocateLease,
relocateNonVoters: relocateNonVoters,
tableDesc: index.Table().(*optTable).desc,
index: index.(*optIndex).idx,
rows: input.(planNode),
subjectReplicas: relocateSubject,
tableDesc: index.Table().(*optTable).desc,
index: index.(*optIndex).idx,
rows: input.(planNode),
}, nil
}

// ConstructAlterRangeRelocate is part of the exec.Factory interface.
func (ef *execFactory) ConstructAlterRangeRelocate(
input exec.Node, relocateLease bool, relocateNonVoters bool, toStoreID int64, fromStoreID int64,
input exec.Node, relocateSubject tree.RelocateSubject, toStoreID int64, fromStoreID int64,
) (exec.Node, error) {
if !ef.planner.ExecCfg().Codec.ForSystemTenant() {
return nil, errorutil.UnsupportedWithMultiTenancy(54250)
}

return &relocateRange{
rows: input.(planNode),
relocateLease: relocateLease,
relocateNonVoters: relocateNonVoters,
toStoreID: roachpb.StoreID(toStoreID),
fromStoreID: roachpb.StoreID(fromStoreID),
rows: input.(planNode),
subjectReplicas: relocateSubject,
toStoreID: roachpb.StoreID(toStoreID),
fromStoreID: roachpb.StoreID(fromStoreID),
}, nil
}

Expand Down
Loading

0 comments on commit 4092554

Please sign in to comment.