opt: cast to identical types for set operations

[RaduBerinde]

This change makes the optbuilder more strict when building set
operations. Previously, it could build expressions which have
corresponding left/right types which are Equivalent(), but not
Identical(). This leads to errors in vectorized execution, when we
e.g. try to union a INT8 with an INT4.

We now make the types on both sides Identical(), adding casts as
necessary. We try to do a best-effort attempt to use the larger
numeric type when possible (e.g. int4->int8, int->float, float->decimal).

Fixes #59148.

Release note (bug fix): fixed execution errors for some queries that
use set operations (UNION / EXCEPT / INTERSECT) where a column has
types of different widths on the two sides (e.g. INT4 vs INT8).

[cockroach-teamcity]

This change is 

[yuzefovich]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/opt/optbuilder/union.go, line 48 at r1 (raw file):

	outScope = inScope.push()

	//// propagateTypesLeft/propagateTypesRight indicate whether we need to wrap

This seems like a leftover?

---

pkg/sql/opt/optbuilder/union.go, line 152 at r1 (raw file):

			// Equivalent but not identical types. Use the type from the left and add
			// a cast on the right-hand side.
			// TODO(radu): perhaps we should do a best effort attempt to choose the

We're actually doing something similar here

cockroach/pkg/sql/colexec/mergejoiner.go

Lines 253 to 271 in fe919cc

	// The types are different and both are numeric, so we need to plan
	// a cast. There is a hierarchy of valid casts:
	// INT2 -> INT4 -> INT8 -> FLOAT -> DECIMAL
	// and the cast is valid if 'fromType' is mentioned before 'toType'
	// in this chain.
	castLeftToRight := false
	switch leftType.Family() {
	case types.IntFamily:
	switch leftType.Width() {
	case 16:
	castLeftToRight = true
	case 32:
	castLeftToRight = !rightType.Identical(types.Int2)
	default:
	castLeftToRight = rightType.Family() != types.IntFamily
	}
	case types.FloatFamily:
	castLeftToRight = rightType.Family() == types.DecimalFamily
	}

Possibly we could extract that code and reuse it here to make the determination.

Postgres does support UNIONing an INT with a DECIMAL:

yuzefovich=# create table t (i int, d decimal);
CREATE TABLE
yuzefovich=# insert into t values (1, 1);
INSERT 0 1
yuzefovich=# select u, pg_typeof(u) from (select i from t union all select d from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | numeric
 1 | numeric
(2 rows)

Also, the postgres upcasts:

yuzefovich=# create table t (a int4, b int8);
CREATE TABLE
yuzefovich=# insert into t values (1, 1);
INSERT 0 1
yuzefovich=# select u, pg_typeof(u) from (select a from t union all select b from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | bigint
 1 | bigint
(2 rows)

yuzefovich=# select u, pg_typeof(u) from (select b from t union all select a from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | bigint
 1 | bigint
(2 rows)

I think it might be worth making us resemble Postgres here since we're touching the code.

[rytaft]

I don't have anything to add beyond the comments from @yuzefovich. I can take another look once you've addressed those. Otherwise looks good.

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

[RaduBerinde]

Updated. We now use the larger (according to Width()) type when possible, and support a few numeric upcasts.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/opt/optbuilder/union.go, line 48 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This seems like a leftover?

Done.

---

pkg/sql/opt/optbuilder/union.go, line 152 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We're actually doing something similar here

cockroach/pkg/sql/colexec/mergejoiner.go

Lines 253 to 271 in fe919cc

	// The types are different and both are numeric, so we need to plan
	// a cast. There is a hierarchy of valid casts:
	// INT2 -> INT4 -> INT8 -> FLOAT -> DECIMAL
	// and the cast is valid if 'fromType' is mentioned before 'toType'
	// in this chain.
	castLeftToRight := false
	switch leftType.Family() {
	case types.IntFamily:
	switch leftType.Width() {
	case 16:
	castLeftToRight = true
	case 32:
	castLeftToRight = !rightType.Identical(types.Int2)
	default:
	castLeftToRight = rightType.Family() != types.IntFamily
	}
	case types.FloatFamily:
	castLeftToRight = rightType.Family() == types.DecimalFamily
	}

Possibly we could extract that code and reuse it here to make the determination.

Postgres does support UNIONing an INT with a DECIMAL:

yuzefovich=# create table t (i int, d decimal);
CREATE TABLE
yuzefovich=# insert into t values (1, 1);
INSERT 0 1
yuzefovich=# select u, pg_typeof(u) from (select i from t union all select d from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | numeric
 1 | numeric
(2 rows)

Also, the postgres upcasts:

yuzefovich=# create table t (a int4, b int8);
CREATE TABLE
yuzefovich=# insert into t values (1, 1);
INSERT 0 1
yuzefovich=# select u, pg_typeof(u) from (select a from t union all select b from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | bigint
 1 | bigint
(2 rows)

yuzefovich=# select u, pg_typeof(u) from (select b from t union all select a from t) as t(u);
 u | pg_typeof 
---+-----------
 1 | bigint
 1 | bigint
(2 rows)

I think it might be worth making us resemble Postgres here since we're touching the code.

Done.

[yuzefovich]

Thanks!

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

---

pkg/sql/opt/optbuilder/union_test.go, line 1 at r2 (raw file):

// Copyright 2020 The Cockroach Authors.

nit: s/2020/2021/.

---

pkg/sql/opt/optbuilder/union_test.go, line 54 at r2 (raw file):

		},
		{
			// At the same scale, we use the left type.

nit: seems like we have different scales here.

[rytaft]

Reviewed 3 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde)

---

pkg/sql/opt/optbuilder/with.go, line 196 at r3 (raw file):

				panic(pgerror.Newf(
					pgcode.DatatypeMismatch,
					"UNION types %s and %s cannot be matched",

[nit] it's possible that the types would be allowed in a normal UNION, just not in a recursive CTE, right? Might be worth mentioning the WITH RECURSIVE part if so.

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

---

pkg/sql/opt/optbuilder/with.go, line 196 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] it's possible that the types would be allowed in a normal UNION, just not in a recursive CTE, right? Might be worth mentioning the WITH RECURSIVE part if so.

Done, and added a test.

[rytaft]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

[RaduBerinde]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[yuzefovich]

Actually, would it be easy to add casts for inputs to a join operator (I'm interested in the merge joiner in particular, but I'm guessing this should also happen during the optbuild stage, before we know which kind of joiner we will be using) so that equality columns are of the same type (in the same cases with numeric types of different widths or families)? This would allow us to get rid of the casts that are planned for the merge joiner in the vectorized engine I mentioned above.

[RaduBerinde]

It would be pretty different - merge joins are generated during exploration. I think probably the xform rule or the execbuilder would be the best place for that. Can you file an issue (and include a pointer to the exec code for mergejoins)?

[yuzefovich]

I think we should backport this. We've seen a couple of sentry reports that could have been caused by this issue (e.g. #61023).