Skip to content

Commit

Permalink
opt: wrap virtual computed column projections in a cast
Browse files Browse the repository at this point in the history 
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct the value of the column - the projection
must produce the same value that would have been written to the table if
the column was a stored computed column. This commit corrects optbuilder
so that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until cockroachdb#81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once cockroachdb#81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes cockroachdb#91817
Informs cockroachdb#81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.
  • Loading branch information
@mgartner
mgartner committed Jun 28, 2023
1 parent 20805cd commit 47d6b6a
Show file tree
Hide file tree
Showing 6 changed files with 248 additions and 34 deletions.
36 changes: 36 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/virtual_columns
View file
Expand Up @@ -1374,3 +1374,39 @@ ALTER TABLE t81675 ADD COLUMN j INT GENERATED ALWAYS AS (i+1) VIRTUAL NOT NULL;

statement ok
DROP TABLE t81675;


# Regression tests for #91817. Assignment casts should be applied to virtual
# computed column projections when the expression type is not identical to the
# column type.
subtest regression_91817

statement ok
CREATE TABLE t91817a (
s STRING,
comp_s "char" AS (s) STORED,
comp_v "char" AS (s) VIRTUAL
);
INSERT INTO t91817a VALUES ('foo')

# The stored and virtual computed columns should have the same value.
query TTT
SELECT * FROM t91817a
----
foo f f

statement ok
CREATE TABLE t91817b (
k INT2 PRIMARY KEY,
v INT2 GENERATED ALWAYS AS (k + 1) VIRTUAL
);
INSERT INTO t91817b VALUES (0)

# This query should not cause an internal error.
query T
SELECT var_pop(v::INT8) OVER ()
FROM t91817b
GROUP BY k, v
HAVING every(true)
----
0
19 changes: 18 additions & 1 deletion pkg/sql/opt/optbuilder/select.go
View file
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/sem/asof"
"github.com/cockroachdb/cockroach/pkg/sql/sem/cast"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
Expand Down Expand Up @@ -866,11 +867,27 @@ func (b *Builder) addComputedColsForTable(tabMeta *opt.TableMeta) {
tableScope.appendOrdinaryColumnsFromTable(tabMeta, &tabMeta.Alias)
}

if texpr := tableScope.resolveAndRequireType(expr, tabCol.DatumType()); texpr != nil {
colType := tabCol.DatumType()
if texpr := tableScope.resolveAndRequireType(expr, colType); texpr != nil {
colID := tabMeta.MetaID.ColumnID(i)
var scalar opt.ScalarExpr
b.factory.FoldingControl().TemporarilyDisallowStableFolds(func() {
scalar = b.buildScalar(texpr, tableScope, nil, nil, nil)
// Add an assignment cast if the types are not identical.
scalarType := scalar.DataType()
if !colType.Identical(scalarType) {
// Assert that an assignment cast is available from the
// expression type to the column type.
if !cast.ValidCast(scalarType, colType, cast.ContextAssignment) {
panic(sqlerrors.NewInvalidAssignmentCastError(
scalarType, colType, string(tabCol.ColName()),
))
}
// TODO(mgartner): This should be an assignment cast, but
// until #81698 is addressed, that could cause reads to
// error after adding a virtual computed column to a table.
scalar = b.factory.ConstructCast(scalar, colType)
}
})
// Check if the expression contains non-immutable operators.
var sharedProps props.Shared
Expand Down
30 changes: 15 additions & 15 deletions pkg/sql/opt/optbuilder/testdata/update
View file
Expand Up @@ -1727,7 +1727,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ └── projections
│ │ │ │ ├── 1.1 [as=a_new:13]
│ │ │ │ └── ARRAY[0.95, NULL, 15::DECIMAL(10,2)] [as=b_new:14]
Expand Down Expand Up @@ -1772,7 +1772,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ └── projections
│ │ │ │ ├── 1.1 [as=a_new:13]
│ │ │ │ └── ARRAY[0.95,NULL,15.00] [as=b_new:14]
Expand Down Expand Up @@ -1812,7 +1812,7 @@ update assn_cast
│ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ └── computed column expressions
│ │ │ └── d_comp:15
│ │ │ └── d:14 + 10.0
│ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ └── projections
│ │ ├── ' ' [as=c_new:19]
│ │ ├── 'foo' [as=qc_new:20]
Expand Down Expand Up @@ -1849,7 +1849,7 @@ update assn_cast
│ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ └── computed column expressions
│ │ │ └── d_comp:15
│ │ │ └── d:14 + 10.0
│ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ └── projections
│ │ ├── ' ' [as=c_new:19]
│ │ ├── 'foo' [as=qc_new:20]
Expand Down Expand Up @@ -1881,7 +1881,7 @@ update assn_cast
│ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ └── computed column expressions
│ │ │ └── d_comp:15
│ │ │ └── d:14 + 10.0
│ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ └── projections
│ │ └── 10::INT2 [as=i_new:19]
│ └── projections
Expand Down Expand Up @@ -1913,7 +1913,7 @@ update assn_cast
│ │ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ │ └── computed column expressions
│ │ │ │ └── d_comp:15
│ │ │ │ └── d:14 + 10.0
│ │ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ │ └── projections
│ │ │ └── 1.45::DECIMAL(10,2) [as=d_new:19]
│ │ └── projections
Expand Down Expand Up @@ -1948,7 +1948,7 @@ update assn_cast
│ │ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ │ └── computed column expressions
│ │ │ │ └── d_comp:15
│ │ │ │ └── d:14 + 10.0
│ │ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ │ └── projections
│ │ │ └── 1.45 [as=d_new:19]
│ │ └── projections
Expand Down Expand Up @@ -1986,7 +1986,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ └── projections
│ │ │ │ ├── 1.1 [as=a_new:13]
│ │ │ │ └── ARRAY[0.95, NULL, 15::DECIMAL(10,2)] [as=b_new:14]
Expand Down Expand Up @@ -2030,7 +2030,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ └── projections
│ │ │ │ ├── 1.1 [as=a_new:13]
│ │ │ │ └── ARRAY[0.95,NULL,15.00] [as=b_new:14]
Expand Down Expand Up @@ -2068,7 +2068,7 @@ update assn_cast
│ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ └── computed column expressions
│ │ │ └── d_comp:15
│ │ │ └── d:14 + 10.0
│ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ └── projections
│ │ ├── ' ' [as=c_new:19]
│ │ └── 10::INT2 [as=i_new:20]
Expand Down Expand Up @@ -2100,7 +2100,7 @@ update assn_cast
│ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ └── computed column expressions
│ │ │ └── d_comp:15
│ │ │ └── d:14 + 10.0
│ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ └── projections
│ │ ├── ' ' [as=c_new:19]
│ │ └── 10::INT2 [as=i_new:20]
Expand Down Expand Up @@ -2137,7 +2137,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ ├── max1-row
│ │ │ │ │ ├── columns: n:13!null
│ │ │ │ │ └── project
Expand Down Expand Up @@ -2185,7 +2185,7 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 decimals.crdb_internal_mvcc_timestamp:11 decimals.tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7 + c:9
│ │ │ │ │ └── (a:7 + c:9)::DECIMAL(10,1)
│ │ │ │ ├── max1-row
│ │ │ │ │ ├── columns: u:13!null "?column?":18!null
│ │ │ │ │ └── project
Expand Down Expand Up @@ -2236,7 +2236,7 @@ update assn_cast
│ │ │ │ ├── columns: c:10 qc:11 i:12 s:13 d:14 d_comp:15 assn_cast.rowid:16!null assn_cast.crdb_internal_mvcc_timestamp:17 assn_cast.tableoid:18
│ │ │ │ └── computed column expressions
│ │ │ │ └── d_comp:15
│ │ │ │ └── d:14 + 10.0
│ │ │ │ └── (d:14 + 10.0)::DECIMAL(10)
│ │ │ ├── max1-row
│ │ │ │ ├── columns: "?column?":24!null "?column?":25!null
│ │ │ │ └── project
Expand Down Expand Up @@ -2283,7 +2283,7 @@ update assn_cast_on_update
│ │ │ │ ├── columns: i:10 i2:11 d:12 d2:13 d_comp:14 b:15 rowid:16!null crdb_internal_mvcc_timestamp:17 tableoid:18
│ │ │ │ └── computed column expressions
│ │ │ │ └── d_comp:14
│ │ │ │ └── d:12
│ │ │ │ └── d:12::DECIMAL(10)
│ │ │ └── projections
│ │ │ ├── 1 [as=i_new:19]
│ │ │ └── true [as=b_new:20]
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/optbuilder/testdata/update-col-cast-bug
View file
Expand Up @@ -30,9 +30,9 @@ with &1 (cte)
│ │ │ │ ├── columns: a:7(int2) b:8(int2) rowid:10(int!null) crdb_internal_mvcc_timestamp:11(decimal) tableoid:12(oid)
│ │ │ │ └── computed column expressions
│ │ │ │ └── t.c:9
│ │ │ │ └── a:7 + b:8 [type=int]
│ │ │ │ └── (a:7 + b:8)::INT2 [type=int2]
│ │ │ └── projections
│ │ │ └── a:7 + b:8 [as=t.c:9, type=int]
│ │ │ └── (a:7 + b:8)::INT2 [as=t.c:9, type=int2]
│ │ └── projections
│ │ └── subquery [as=a_new:16, type=int2]
│ │ └── max1-row
Expand Down

0 comments on commit 47d6b6a

Please sign in to comment.