Skip to content

Commit

Permalink
opt: eliminate assignment casts with identical source and target types
Browse files Browse the repository at this point in the history
The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see cockroachdb#73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

Fixes cockroachdb#73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.
  • Loading branch information
mgartner committed Dec 14, 2021
1 parent d4596bf commit 9a13053
Show file tree
Hide file tree
Showing 17 changed files with 198 additions and 169 deletions.
63 changes: 63 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/cast
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ CREATE TABLE assn_cast (
qc "char",
b BIT,
i INT,
i2 INT2,
f4 FLOAT4,
t timestamp,
d DECIMAL(10, 0),
a DECIMAL(10, 0)[],
Expand Down Expand Up @@ -141,6 +143,33 @@ EXECUTE insert_i('1')
statement error value type string doesn't match type int of column \"i\"
INSERT INTO assn_cast(i) VALUES ('1'::STRING)

statement error integer out of range for type int2
INSERT INTO assn_cast(i2) VALUES (999999999)

statement ok
PREPARE insert_i2 AS INSERT INTO assn_cast(i2) VALUES ($1)

statement error integer out of range for type int2
EXECUTE insert_i2(99999999)

statement ok
INSERT INTO assn_cast(f4) VALUES (18754999.99)

statement ok
PREPARE insert_f4 AS INSERT INTO assn_cast(f4) VALUES ($1)

statement ok
EXECUTE insert_f4(18754999.99)

# TODO(mgartner): Values are not correctly truncated when cast to FLOAT4, either
# in an assignment or explicit context. The columns should have a value of
# 1.8755e+07. See #73743.
query F
SELECT f4 FROM assn_cast WHERE f4 IS NOT NULL
----
1.875499999e+07
1.875499999e+07

statement ok
INSERT INTO assn_cast(t) VALUES ('1970-01-01'::timestamptz)

Expand Down Expand Up @@ -350,3 +379,37 @@ SELECT '-'::regclass, '-'::regclass::oid,
'-'::regrole, '-'::regrole::oid
----
- 0 - 0 - 0 - 0 - 0 - 0

# Regression test for #73450. Eliding casts should not cause incorrect results.
subtest regression_73450

statement ok
PREPARE s73450_a AS SELECT $1::INT2

statement error integer out of range for type int2
EXECUTE s73450_a(999999)

statement ok
PREPARE s73450_b AS SELECT $1::CHAR

query T
EXECUTE s73450_b('foo')
----
f

statement ok
CREATE TABLE t73450 (c CHAR);
INSERT INTO t73450 VALUES ('f')

query T
SELECT * FROM t73450 WHERE c = 'foo'::CHAR
----
f

statement ok
PREPARE s73450_c AS SELECT * FROM t73450 WHERE c = $1::CHAR

query T
EXECUTE s73450_c('foo')
----
f
18 changes: 9 additions & 9 deletions pkg/sql/opt/exec/execbuilder/testdata/insert
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,8 @@ vectorized: true
│ columns: (x, v, rowid_default)
│ estimated row count: 10 (missing stats)
│ render rowid_default: unique_rowid()
│ render x: crdb_internal.assignment_cast(x, NULL::INT8)
│ render v: crdb_internal.assignment_cast(v, NULL::INT8)
│ render x: x
│ render v: v
└── • top-k
│ columns: (x, v)
Expand Down Expand Up @@ -355,8 +355,8 @@ vectorized: true
│ columns: (x, v, rowid_default)
│ estimated row count: 1 (missing stats)
│ render rowid_default: unique_rowid()
│ render x: crdb_internal.assignment_cast(x, NULL::INT8)
│ render v: crdb_internal.assignment_cast(v, NULL::INT8)
│ render x: x
│ render v: v
└── • scan
columns: (x, v)
Expand Down Expand Up @@ -471,8 +471,8 @@ vectorized: true
│ columns: (length, "?column?", rowid_default)
│ estimated row count: 10 (missing stats)
│ render rowid_default: unique_rowid()
│ render length: crdb_internal.assignment_cast(length, NULL::INT8)
│ render ?column?: crdb_internal.assignment_cast("?column?", NULL::INT8)
│ render length: length
│ render ?column?: "?column?"
└── • top-k
│ columns: (length, "?column?", column12)
Expand Down Expand Up @@ -600,9 +600,9 @@ vectorized: true
│ columns: (a, b, c, rowid_default)
│ estimated row count: 1,000 (missing stats)
│ render rowid_default: unique_rowid()
│ render a: crdb_internal.assignment_cast(a, NULL::INT8)
│ render b: crdb_internal.assignment_cast(b, NULL::INT8)
│ render c: crdb_internal.assignment_cast(c, NULL::INT8)
│ render a: a
│ render b: b
│ render c: c
└── • scan
columns: (a, b, c)
Expand Down
24 changes: 8 additions & 16 deletions pkg/sql/opt/norm/rules/scalar.opt
Original file line number Diff line number Diff line change
Expand Up @@ -58,26 +58,18 @@ $item
=>
(SimplifyCoalesce $args)

# EliminateCast discards the cast operator if its input already has a type
# that's identical to the desired static type.
# EliminateCast discards a cast or assignment cast operator if its input already
# has a type that's identical to the desired static type.
#
# Note that CastExpr removes unnecessary casts during type-checking; this rule
# can still be helpful if some other rule creates an unnecessary CastExpr.
[EliminateCast, Normalize]
(Cast $input:* $targetTyp:* & (HasColType $input $targetTyp))
=>
$input

# EliminateAssignmentCast discards the assignment cast operator if the input
# type and the target type are identical and the assignment cast is guaranteed
# to be a no-op. See AssignmentCastIsNoop for an explanation of what makes an
# assignment cast a no-op.
[EliminateAssignmentCast, Normalize]
(AssignmentCast
#
# EliminateCast is marked as high-priority so that it matches before
# FoldAssignmentCast.
[EliminateCast, Normalize, HighPriority]
(Cast | AssignmentCast
$input:*
$targetTyp:* &
(HasColType $input $targetTyp) &
(AssignmentCastIsNoop $targetTyp)
$targetTyp:* & (HasColType $input $targetTyp)
)
=>
$input
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/norm/testdata/rules/fold_constants
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ insert assn_cast

# Do not fold non-constants even if they have an identical type as the target
# type.
norm expect-not=FoldAssignmentCast
norm expect-not=FoldAssignmentCast disable=EliminateCast
INSERT INTO assn_cast(c) VALUES ($1::CHAR)
----
insert assn_cast
Expand Down
47 changes: 10 additions & 37 deletions pkg/sql/opt/norm/testdata/rules/groupby
Original file line number Diff line number Diff line change
Expand Up @@ -953,17 +953,14 @@ insert xy
└── limit
├── columns: y:9!null y_default:10
├── cardinality: [0 - 1]
├── immutable
├── key: ()
├── fd: ()-->(9,10)
├── anti-join (hash)
│ ├── columns: y:9!null y_default:10
│ ├── immutable
│ ├── fd: ()-->(9,10)
│ ├── limit hint: 1.00
│ ├── project
│ │ ├── columns: y_default:10 y:9!null
│ │ ├── immutable
│ │ ├── fd: ()-->(9,10)
│ │ ├── select
│ │ │ ├── columns: xy.y:6!null
Expand All @@ -974,8 +971,7 @@ insert xy
│ │ │ └── xy.y:6 = 0 [outer=(6), constraints=(/6: [/0 - /0]; tight), fd=()-->(6)]
│ │ └── projections
│ │ ├── CAST(NULL AS INT8) [as=y_default:10]
│ │ └── assignment-cast: INT8 [as=y:9, outer=(6), immutable]
│ │ └── xy.y:6
│ │ └── xy.y:6 [as=y:9, outer=(6)]
│ ├── scan xy
│ │ ├── columns: x:11!null
│ │ └── key: (11)
Expand Down Expand Up @@ -1056,16 +1052,13 @@ insert xy
└── upsert-distinct-on
├── columns: y:9 y_default:10
├── grouping columns: y:9
├── immutable
├── lax-key: (9)
├── fd: ()-->(9,10)
├── anti-join (hash)
│ ├── columns: y:9 y_default:10
│ ├── immutable
│ ├── fd: ()-->(9,10)
│ ├── project
│ │ ├── columns: y_default:10 y:9
│ │ ├── immutable
│ │ ├── fd: ()-->(9,10)
│ │ ├── select
│ │ │ ├── columns: xy.y:6
Expand All @@ -1076,8 +1069,7 @@ insert xy
│ │ │ └── xy.y:6 IS NULL [outer=(6), constraints=(/6: [/NULL - /NULL]; tight), fd=()-->(6)]
│ │ └── projections
│ │ ├── CAST(NULL AS INT8) [as=y_default:10]
│ │ └── assignment-cast: INT8 [as=y:9, outer=(6), immutable]
│ │ └── xy.y:6
│ │ └── xy.y:6 [as=y:9, outer=(6)]
│ ├── scan xy
│ │ ├── columns: x:11!null
│ │ └── key: (11)
Expand Down Expand Up @@ -1687,17 +1679,14 @@ insert a
└── limit
├── columns: "?column?":17!null i:18!null "?column?":19!null f_default:20 j_default:21
├── cardinality: [0 - 1]
├── immutable
├── key: ()
├── fd: ()-->(17-21)
├── anti-join (hash)
│ ├── columns: "?column?":17!null i:18!null "?column?":19!null f_default:20 j_default:21
│ ├── immutable
│ ├── fd: ()-->(17-21)
│ ├── limit hint: 1.00
│ ├── project
│ │ ├── columns: f_default:20 j_default:21 "?column?":17!null i:18!null "?column?":19!null
│ │ ├── immutable
│ │ ├── fd: ()-->(17-21)
│ │ ├── select
│ │ │ ├── columns: a.i:9!null
Expand All @@ -1710,8 +1699,7 @@ insert a
│ │ ├── CAST(NULL AS FLOAT8) [as=f_default:20]
│ │ ├── CAST(NULL AS JSONB) [as=j_default:21]
│ │ ├── 1 [as="?column?":17]
│ │ ├── assignment-cast: INT8 [as=i:18, outer=(9), immutable]
│ │ │ └── a.i:9
│ │ ├── a.i:9 [as=i:18, outer=(9)]
│ │ └── 'foo' [as="?column?":19]
│ ├── select
│ │ ├── columns: a.i:23!null s:25!null
Expand Down Expand Up @@ -2465,18 +2453,8 @@ insert a
│ │ │ │ │ ├── columns: column1:12 column2:13!null column3:14!null column4:15!null
│ │ │ │ │ ├── cardinality: [2 - 2]
│ │ │ │ │ ├── volatile
│ │ │ │ │ ├── tuple
│ │ │ │ │ │ ├── assignment-cast: INT8
│ │ │ │ │ │ │ └── unique_rowid()
│ │ │ │ │ │ ├── 'foo'
│ │ │ │ │ │ ├── 1
│ │ │ │ │ │ └── 1.0
│ │ │ │ │ └── tuple
│ │ │ │ │ ├── assignment-cast: INT8
│ │ │ │ │ │ └── unique_rowid()
│ │ │ │ │ ├── 'bar'
│ │ │ │ │ ├── 2
│ │ │ │ │ └── 2.0
│ │ │ │ │ ├── (unique_rowid(), 'foo', 1, 1.0)
│ │ │ │ │ └── (unique_rowid(), 'bar', 2, 2.0)
│ │ │ │ └── projections
│ │ │ │ └── CAST(NULL AS JSONB) [as=j_default:16]
│ │ │ ├── scan a
Expand Down Expand Up @@ -2526,27 +2504,22 @@ insert a
└── upsert-distinct-on
├── columns: i:16!null "?column?":17!null i:18!null f_default:19 j_default:20
├── grouping columns: i:18!null
├── immutable
├── key: (18)
├── fd: ()-->(17,19,20), (18)-->(16,17,19,20)
├── fd: ()-->(17,19,20), (16)==(18), (18)==(16), (18)-->(16,17,19,20)
├── anti-join (hash)
│ ├── columns: i:16!null "?column?":17!null i:18!null f_default:19 j_default:20
│ ├── immutable
│ ├── fd: ()-->(17,19,20)
│ ├── fd: ()-->(17,19,20), (16)==(18), (18)==(16)
│ ├── project
│ │ ├── columns: f_default:19 j_default:20 i:16!null "?column?":17!null i:18!null
│ │ ├── immutable
│ │ ├── fd: ()-->(17,19,20)
│ │ ├── fd: ()-->(17,19,20), (16)==(18), (18)==(16)
│ │ ├── scan a
│ │ │ └── columns: a.i:9!null
│ │ └── projections
│ │ ├── CAST(NULL AS FLOAT8) [as=f_default:19]
│ │ ├── CAST(NULL AS JSONB) [as=j_default:20]
│ │ ├── assignment-cast: INT8 [as=i:16, outer=(9), immutable]
│ │ │ └── a.i:9
│ │ ├── a.i:9 [as=i:16, outer=(9)]
│ │ ├── 'foo' [as="?column?":17]
│ │ └── assignment-cast: INT8 [as=i:18, outer=(9), immutable]
│ │ └── a.i:9
│ │ └── a.i:9 [as=i:18, outer=(9)]
│ ├── select
│ │ ├── columns: a.i:22!null s:24!null
│ │ ├── key: (22)
Expand Down
17 changes: 6 additions & 11 deletions pkg/sql/opt/norm/testdata/rules/project
Original file line number Diff line number Diff line change
Expand Up @@ -1103,10 +1103,8 @@ insert assn_cast
│ │ └── $1
│ ├── assignment-cast: "char"
│ │ └── $2
│ ├── assignment-cast: INT8
│ │ └── $3
│ └── assignment-cast: STRING
│ └── $4
│ ├── $3
│ └── $4
└── projections
└── unique_rowid() [as=rowid_default:16, volatile]

Expand Down Expand Up @@ -1158,23 +1156,20 @@ insert assn_cast
├── cardinality: [0 - 0]
├── volatile, mutations
└── project
├── columns: c_default:12 qc_default:13 rowid_default:14 x:10!null text:11!null
├── columns: c_default:12 qc_default:13 rowid_default:14 text:11!null x:10!null
├── cardinality: [2 - 2]
├── volatile
├── fd: ()-->(12,13)
├── fd: ()-->(12,13), (10)-->(11)
├── values
│ ├── columns: column1:8!null
│ ├── columns: x:10!null
│ ├── cardinality: [2 - 2]
│ ├── (1,)
│ └── (2,)
└── projections
├── CAST(NULL AS CHAR) [as=c_default:12]
├── CAST(NULL AS "char") [as=qc_default:13]
├── unique_rowid() [as=rowid_default:14, volatile]
├── assignment-cast: INT8 [as=x:10, outer=(8), immutable]
│ └── column1:8
└── assignment-cast: STRING [as=text:11, outer=(8), immutable]
└── (column1:8 + 1)::STRING
└── (x:10 + 1)::STRING [as=text:11, outer=(10), immutable]

# Do not match if there are no assignment cast projections.
norm expect-not=PushAssignmentCastsIntoValues
Expand Down
Loading

0 comments on commit 9a13053

Please sign in to comment.