opt: eliminate assignment casts with identical source and target types

[mgartner]

sql: remove type modifiers from placeholder types

The width and precision of placeholder values are not known during
PREPARE, therefore we remove any type modifiers from placeholder types
during type checking so that a value of any width will fit within the
palceholder type.

Note that this change is similar to the changes made in #70722 to
placeholder type checking that were later reverted in #72793. In #70722
the type OIDs of placeholders could be altered, e.g., a placeholder
originally with type INT2 would be converted to an INT. In this
commit, type OIDs of placeholders are not changing, only type modifiers
are.

This commit should allow some logic added in #72793 to be simplified or
removed entirely.

Release note: None

opt: eliminate assignment casts with identical source and target types

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 NumVals 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 sql: truncate floats to FLOAT4 correctly #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.

4. Type modifiers are now correctly omitted from array content types
   when arrays contain constants.

Fixes #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.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Fingers crossed this works without issues :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @otan)

---

pkg/sql/opt/norm/rules/scalar.opt, line 80 at r2 (raw file):

    $targetTyp:* &
        (HasColType $input $targetTyp) &
        (AssignmentCastIsNoop $targetTyp)

[nit] We should remove this function

---

pkg/sql/sem/tree/type_check.go, line 1621 at r1 (raw file):

			// later, we cast the input value (whose type is the expected type) to the
			// desired type here.
			typ = desired

This seems sketchy - do you think this still makes sense? Does the cast the comment mentions still exist? (AFAICT no, it used to exist in the old equivalent of AssignPlaceholders) By the way, I'm surprised AssignPlaceholders doesn't make any assertions w.r.t the type.

---

pkg/sql/sem/tree/type_check.go, line 1549 at r2 (raw file):

	desiredParam := types.Any
	if desired.Family() == types.ArrayFamily {
		desiredParam = desired.ArrayContents().WithoutTypeModifiers()

This seems random, can you explain in a comment?

[otan]

Reviewed 8 of 8 files at r1, 1 of 17 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @otan, and @RaduBerinde)

---

pkg/sql/sem/tree/type_check.go, line 1549 at r2 (raw file):

Previously, RaduBerinde wrote…

This seems random, can you explain in a comment?

mmm yes +1

---

pkg/sql/types/types.go, line 1266 at r1 (raw file):

			return t
		}
		newT := *t

my fear here is that we'll miss something when adding a new type. should we explicitly either catch all other families and panic otherwise, and/or use MakeScalar/some mapping from Oid to "default type" (e.g. types.Int, types.TimestampTZ) which should make the "empty" versions of these types?

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @RaduBerinde)

---

pkg/sql/opt/norm/rules/scalar.opt, line 80 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] We should remove this function

Good catch. Done.

---

pkg/sql/sem/tree/type_check.go, line 1621 at r1 (raw file):

This seems sketchy - do you think this still makes sense? Does the cast the comment mentions still exist?

I believe this comment refers to the cast added in Placeholder.Eval. I'm not convinced it's necessary anymore, and in some cases may not be valid (notice the TODO). I can explore removing in a future commit.

By the way, I'm surprised AssignPlaceholders doesn't make any assertions w.r.t the type.

The assertions happen earlier on in planner.fillInPlaceholders. schemaexpr.SanitizeVarFreeExpr calls tree.TypeCheck which eventually calls Constant.ResolveAsType on the placeholder values. Notice the change in the second commit to NumVal.ResolveAsType which adds missing assertions for integer constants.

I get the feeling all this type-related logic at the conn_executor level is from pre-CBO times and it doesn't really belong there anymore. But I don't have enough experience with the conn_executor to have strong opinions yet.

---

pkg/sql/sem/tree/type_check.go, line 1549 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

mmm yes +1

I've reverted this in favor of a change in typeCheckSameTypedConsts, where I've explained with a comment. The goal is to correctly type the literal array below as DECIMAL[], and not DECIMAL(10,0)[] which causes an assignment cast to be elided and an incorrect value inserted.

CREATE TABLE t (a DECIMAL(10,0)[]);
INSERT INTO t VALUES (ARRAY[2.88, NULL, 15]);

---

pkg/sql/types/types.go, line 1266 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

my fear here is that we'll miss something when adding a new type. should we explicitly either catch all other families and panic otherwise, and/or use MakeScalar/some mapping from Oid to "default type" (e.g. types.Int, types.TimestampTZ) which should make the "empty" versions of these types?

Good point. I don't think catching all families is sufficient - if a type were added to a family we'd have a similar problem. I've change this function to use type OIDs, let me know what you think.

[otan]

Reviewed 11 of 19 files at r3, 1 of 18 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @RaduBerinde)

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @RaduBerinde)

---

pkg/sql/types/types.go, line 1266 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Good point. I don't think catching all families is sufficient - if a type were added to a family we'd have a similar problem. I've change this function to use type OIDs, let me know what you think.

It already caught the new void type 😉

[mgartner]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)