Skip to content

Commit 51a51d8

Browse files
spilchenspilchen
committed
sql/schemachanger: move column compute expression to separate DSC element
Previously, the column compute expression was part of the ColumnType element, which made it difficult to change the compute expression without affecting the other fields in ColumnType. To address this, especially for the upcoming work on adding a temporary compute expression during column type changes that require a backfill (in #127014), we’re moving the compute expression into its own separate element, similar to how default expressions (ColumnDefaultExpression) and ON UPDATE expressions (ColumnOnUpdateExpression) are handled. Key points to note about this change: - Version Gate: The new element must be protected by a version gate. For older versions, we’ll continue using the old compute expression field in ColumnType. There was one instance where the current cluster version wasn’t accessible, so I added a field in ElementCreationMetadata to track if the ColumnType was created in 24.3. This acts as the version gate. - IsVirtual Attribute: Although logically, the IsVirtual attribute should belong to ColumnComputeExpression, I left it in ColumnType. During the ADD COLUMN process, we set up the column family for the new column only if it’s not virtual. Moving IsVirtual to ColumnComputeExpression would require a dependency rule to ensure the ColumnComputeExpression is applied before ColumnType to set up the column family correctly. - Dropping a Compute Column: When dropping a computed column, I leave a NULL expression in the column descriptor. If the expression is removed entirely, it triggers validation logic that a virtual column must have a compute expression. We will need to revisit this, especially for the ALTER TABLE .. TYPE use case and future support for ALTER TABLE .. DROP EXPRESSION. Epic: CRDB-25314 Informs: #127014 Release note: None
1 parent 5dbc891 commit 51a51d8

File tree

107 files changed

+1327
-539
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

107 files changed

+1327
-539
lines changed

‎pkg/ccl/schemachangerccl/testdata/decomp/multiregion‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ ElementState:
200200
computeExpr: null
201201
elementCreationMetadata:
202202
in231OrLater: true
203+
in243OrLater: true
203204
familyId: 0
204205
isNullable: true
205206
isVirtual: false
@@ -228,6 +229,7 @@ ElementState:
228229
computeExpr: null
229230
elementCreationMetadata:
230231
in231OrLater: true
232+
in243OrLater: true
231233
familyId: 0
232234
isNullable: false
233235
isVirtual: false
@@ -256,6 +258,7 @@ ElementState:
256258
computeExpr: null
257259
elementCreationMetadata:
258260
in231OrLater: true
261+
in243OrLater: true
259262
familyId: 0
260263
isNullable: true
261264
isVirtual: false
@@ -284,6 +287,7 @@ ElementState:
284287
computeExpr: null
285288
elementCreationMetadata:
286289
in231OrLater: true
290+
in243OrLater: true
287291
familyId: 0
288292
isNullable: true
289293
isVirtual: false
@@ -497,6 +501,7 @@ ElementState:
497501
computeExpr: null
498502
elementCreationMetadata:
499503
in231OrLater: true
504+
in243OrLater: true
500505
familyId: 0
501506
isNullable: true
502507
isVirtual: false
@@ -525,6 +530,7 @@ ElementState:
525530
computeExpr: null
526531
elementCreationMetadata:
527532
in231OrLater: true
533+
in243OrLater: true
528534
familyId: 0
529535
isNullable: false
530536
isVirtual: false
@@ -553,6 +559,7 @@ ElementState:
553559
computeExpr: null
554560
elementCreationMetadata:
555561
in231OrLater: true
562+
in243OrLater: true
556563
familyId: 0
557564
isNullable: true
558565
isVirtual: false
@@ -581,6 +588,7 @@ ElementState:
581588
computeExpr: null
582589
elementCreationMetadata:
583590
in231OrLater: true
591+
in243OrLater: true
584592
familyId: 0
585593
isNullable: true
586594
isVirtual: false
@@ -835,6 +843,7 @@ ElementState:
835843
computeExpr: null
836844
elementCreationMetadata:
837845
in231OrLater: true
846+
in243OrLater: true
838847
familyId: 0
839848
isNullable: false
840849
isVirtual: false
@@ -864,6 +873,7 @@ ElementState:
864873
computeExpr: null
865874
elementCreationMetadata:
866875
in231OrLater: true
876+
in243OrLater: true
867877
familyId: 0
868878
isNullable: false
869879
isVirtual: false
@@ -892,6 +902,7 @@ ElementState:
892902
computeExpr: null
893903
elementCreationMetadata:
894904
in231OrLater: true
905+
in243OrLater: true
895906
familyId: 0
896907
isNullable: true
897908
isVirtual: false
@@ -920,6 +931,7 @@ ElementState:
920931
computeExpr: null
921932
elementCreationMetadata:
922933
in231OrLater: true
934+
in243OrLater: true
923935
familyId: 0
924936
isNullable: true
925937
isVirtual: false
@@ -948,6 +960,7 @@ ElementState:
948960
computeExpr: null
949961
elementCreationMetadata:
950962
in231OrLater: true
963+
in243OrLater: true
951964
familyId: 0
952965
isNullable: true
953966
isVirtual: false

‎pkg/ccl/schemachangerccl/testdata/decomp/partitioning‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ ElementState:
131131
computeExpr: null
132132
elementCreationMetadata:
133133
in231OrLater: true
134+
in243OrLater: true
134135
familyId: 0
135136
isNullable: false
136137
isVirtual: false
@@ -159,6 +160,7 @@ ElementState:
159160
computeExpr: null
160161
elementCreationMetadata:
161162
in231OrLater: true
163+
in243OrLater: true
162164
familyId: 0
163165
isNullable: false
164166
isVirtual: false
@@ -187,6 +189,7 @@ ElementState:
187189
computeExpr: null
188190
elementCreationMetadata:
189191
in231OrLater: true
192+
in243OrLater: true
190193
familyId: 0
191194
isNullable: true
192195
isVirtual: false
@@ -215,6 +218,7 @@ ElementState:
215218
computeExpr: null
216219
elementCreationMetadata:
217220
in231OrLater: true
221+
in243OrLater: true
218222
familyId: 0
219223
isNullable: true
220224
isVirtual: false
@@ -243,6 +247,7 @@ ElementState:
243247
computeExpr: null
244248
elementCreationMetadata:
245249
in231OrLater: true
250+
in243OrLater: true
246251
familyId: 0
247252
isNullable: true
248253
isVirtual: false
@@ -534,6 +539,7 @@ ElementState:
534539
computeExpr: null
535540
elementCreationMetadata:
536541
in231OrLater: true
542+
in243OrLater: true
537543
familyId: 0
538544
isNullable: false
539545
isVirtual: false
@@ -562,6 +568,7 @@ ElementState:
562568
computeExpr: null
563569
elementCreationMetadata:
564570
in231OrLater: true
571+
in243OrLater: true
565572
familyId: 0
566573
isNullable: true
567574
isVirtual: false
@@ -590,6 +597,7 @@ ElementState:
590597
computeExpr: null
591598
elementCreationMetadata:
592599
in231OrLater: true
600+
in243OrLater: true
593601
familyId: 0
594602
isNullable: true
595603
isVirtual: false
@@ -618,6 +626,7 @@ ElementState:
618626
computeExpr: null
619627
elementCreationMetadata:
620628
in231OrLater: true
629+
in243OrLater: true
621630
familyId: 0
622631
isNullable: true
623632
isVirtual: false

‎pkg/sql/catalog/redact/redact.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,8 @@ func redactElement(element scpb.Element) error {
168168
if e.ComputeExpr != nil {
169169
return redactExpr(&e.ComputeExpr.Expr)
170170
}
171+
case *scpb.ColumnComputeExpression:
172+
return redactExpr(&e.Expression.Expr)
171173
case *scpb.FunctionBody:
172174
return redactFunctionBodyStr(e.Lang.Lang, &e.Body)
173175
}

‎pkg/sql/opt/exec/execbuilder/testdata/show_trace‎

Lines changed: 2 additions & 2 deletions
Large diffs are not rendered by default.

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,16 @@ func alterTableAddColumn(
175175
))
176176
}
177177
if desc.IsComputed() {
178-
expr := b.ComputedColumnExpression(tbl, d)
179-
spec.colType.ComputeExpr = b.WrapExpression(tbl.TableID, expr)
178+
expr := b.WrapExpression(tbl.TableID, b.ComputedColumnExpression(tbl, d))
179+
if spec.colType.ElementCreationMetadata.In_24_3OrLater {
180+
spec.compute = &scpb.ColumnComputeExpression{
181+
TableID: tbl.TableID,
182+
ColumnID: spec.col.ColumnID,
183+
Expression: *expr,
184+
}
185+
} else {
186+
spec.colType.ComputeExpr = expr
187+
}
180188
if desc.Virtual {
181189
b.IncrementSchemaChangeAddColumnQualificationCounter("virtual")
182190
} else {
@@ -270,6 +278,7 @@ type addColumnSpec struct {
270278
colType *scpb.ColumnType
271279
def *scpb.ColumnDefaultExpression
272280
onUpdate *scpb.ColumnOnUpdateExpression
281+
compute *scpb.ColumnComputeExpression
273282
comment *scpb.ColumnComment
274283
unique bool
275284
notNull bool
@@ -298,6 +307,9 @@ func addColumn(b BuildCtx, spec addColumnSpec, n tree.NodeFormatter) (backing *s
298307
if spec.onUpdate != nil {
299308
b.Add(spec.onUpdate)
300309
}
310+
if spec.compute != nil {
311+
b.Add(spec.compute)
312+
}
301313
if spec.comment != nil {
302314
b.Add(spec.comment)
303315
}
@@ -307,7 +319,7 @@ func addColumn(b BuildCtx, spec addColumnSpec, n tree.NodeFormatter) (backing *s
307319
}
308320

309321
inflatedChain := getInflatedPrimaryIndexChain(b, spec.tbl.TableID)
310-
if spec.def == nil && spec.colType.ComputeExpr == nil {
322+
if spec.def == nil && spec.colType.ComputeExpr == nil && spec.compute == nil {
311323
// Optimization opportunity: if we were to add a new column without default
312324
// value nor computed expression, then we can just add the column to existing
313325
// non-nil primary indexes without actually backfilling any data. This is

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,8 @@ func ensureColCanBeUsedInOutboundFK(
809809
// actions that would try to change the computed value. Computed values cannot
810810
// be altered directly, so attempts to set them to NULL or a DEFAULT value are
811811
// blocked.
812-
if colTypeElem.ComputeExpr != nil && actions.HasDisallowedActionForComputedFKCol() {
812+
computeExpr := retrieveColumnComputeExpression(b, tableID, columnID)
813+
if computeExpr != nil && actions.HasDisallowedActionForComputedFKCol() {
813814
panic(sqlerrors.NewInvalidActionOnComputedFKColumnError(actions.HasUpdateAction()))
814815
}
815816
}

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func alterTableSetDefault(
3838
panicIfSystemColumn(col, t.Column.String())
3939

4040
// Block disallowed operations on computed columns.
41-
panicIfComputedColumn(tn.ObjectName, colType, t.Column.String(), t.Default)
41+
panicIfComputedColumn(b, tn.ObjectName, colType, t.Column.String(), t.Default)
4242

4343
// For DROP DEFAULT.
4444
if t.Default == nil {
@@ -148,9 +148,12 @@ func sanitizeColumnExpression(
148148
}
149149

150150
// panicIfComputedColumn blocks disallowed operations on computed columns.
151-
func panicIfComputedColumn(tn tree.Name, col *scpb.ColumnType, colName string, def tree.Expr) {
151+
func panicIfComputedColumn(
152+
b BuildCtx, tn tree.Name, col *scpb.ColumnType, colName string, def tree.Expr,
153+
) {
154+
computeExpr := retrieveColumnComputeExpression(b, col.TableID, col.ColumnID)
152155
// Block setting a column default if the column is computed.
153-
if col.ComputeExpr != nil {
156+
if computeExpr != nil {
154157
// Block dropping a computed col "default" as well.
155158
if def == nil {
156159
panic(pgerror.Newf(

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ func walkColumnDependencies(
299299
} else {
300300
columnDeps.Add(elt.ColumnID)
301301
}
302+
case *scpb.ColumnComputeExpression:
303+
if elt.ColumnID == col.ColumnID {
304+
fn(e, op, objType)
305+
} else {
306+
columnDeps.Add(elt.ColumnID)
307+
}
302308
case *scpb.SequenceOwner:
303309
fn(e, op, objType)
304310
sequenceDeps.Add(elt.SequenceID)

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -736,13 +736,23 @@ func maybeCreateAndAddShardCol(
736736
TableID: tbl.TableID,
737737
ColumnID: shardColID,
738738
TypeT: newTypeT(types.Int),
739-
ComputeExpr: b.WrapExpression(tbl.TableID, parsedExpr),
740739
IsVirtual: true,
741740
IsNullable: false,
742741
ElementCreationMetadata: scdecomp.NewElementCreationMetadata(b.EvalCtx().Settings.Version.ActiveVersion(b)),
743742
},
744743
notNull: true,
745744
}
745+
wexpr := b.WrapExpression(tbl.TableID, parsedExpr)
746+
if spec.colType.ElementCreationMetadata.In_24_3OrLater {
747+
spec.compute = &scpb.ColumnComputeExpression{
748+
TableID: tbl.TableID,
749+
ColumnID: shardColID,
750+
Expression: *wexpr,
751+
}
752+
} else {
753+
spec.colType.ComputeExpr = wexpr
754+
}
755+
746756
backing := addColumn(b, spec, n)
747757
// Create a new check constraint for the hash sharded index column.
748758
checkConstraintBucketValues := strings.Builder{}
@@ -843,8 +853,9 @@ func maybeCreateVirtualColumnForIndex(
843853
// if it's a virtual column created for an index expression.
844854
scpb.ForEachColumnType(elts, func(current scpb.Status, target scpb.TargetStatus, e *scpb.ColumnType) {
845855
column := mustRetrieveColumnElem(b, e.TableID, e.ColumnID)
846-
if target == scpb.ToPublic && e.ComputeExpr != nil && e.IsVirtual && column.IsInaccessible {
847-
otherExpr, err := parser.ParseExpr(string(e.ComputeExpr.Expr))
856+
computeExpr := retrieveColumnComputeExpression(b, e.TableID, e.ColumnID)
857+
if target == scpb.ToPublic && computeExpr != nil && e.IsVirtual && column.IsInaccessible {
858+
otherExpr, err := parser.ParseExpr(string(computeExpr.Expr))
848859
if err != nil {
849860
panic(err)
850861
}

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ func CreateSequence(b BuildCtx, n *tree.CreateSequence) {
168168
TableID: sequenceID,
169169
ColumnID: tabledesc.SequenceColumnID,
170170
TypeT: newTypeT(types.Int),
171-
ElementCreationMetadata: &scpb.ElementCreationMetadata{In_23_1OrLater: true},
171+
ElementCreationMetadata: scdecomp.NewElementCreationMetadata(b.EvalCtx().Settings.Version.ActiveVersion(b)),
172172
})
173173
b.Add(&scpb.ColumnNotNull{
174174
TableID: sequenceID,

0 commit comments

Comments
 (0)