From f46f3770cbfee8e0d79a1b148d47dec0dc51adfb Mon Sep 17 00:00:00 2001
From: Marcus Gartner <marcus@cockroachlabs.com>
Date: Thu, 20 Jan 2022 10:43:26 -0500
Subject: [PATCH] opt: do not add invalid casts to match types in set
 operations

The optbuilder adds casts in order to match non-identical left/right
types of set operations like UNION (see #60560). This change prevents
the optbuilder from adding casts that are invalid, which caused internal
errors. Now, a user error is thrown if no such cast exists from the left
or right input type to the output type.

Release note (bug fix): A bug has been fixed that caused internal errors
in queries with set operations, like UNION, when corresponding columns
on either side of the set operation were not the same. This error only
occurred with a limited set of types. This bug is present in versions
20.2.6+, 21.1.0+, and 21.2.0+.
---
 pkg/cmd/roachtest/tests/sqlsmith.go   |  3 +--
 pkg/sql/opt/optbuilder/testdata/union | 14 +++++++++++++
 pkg/sql/opt/optbuilder/union.go       | 29 ++++++++++++++++++---------
 pkg/sql/opt/optbuilder/union_test.go  |  6 ++++++
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/pkg/cmd/roachtest/tests/sqlsmith.go b/pkg/cmd/roachtest/tests/sqlsmith.go
index 2b3b0eb35ab6..24c3767b8ac6 100644
--- a/pkg/cmd/roachtest/tests/sqlsmith.go
+++ b/pkg/cmd/roachtest/tests/sqlsmith.go
@@ -246,11 +246,10 @@ INSERT INTO seed_mr_table DEFAULT VALUES;`, regionList[0]),
 				es := err.Error()
 				if strings.Contains(es, "internal error") {
 					// TODO(yuzefovich): we temporarily ignore internal errors
-					// that are because of #40929 and #70831.
+					// that are because of #40929.
 					var expectedError bool
 					for _, exp := range []string{
 						"could not parse \"0E-2019\" as type decimal",
-						"no volatility for cast tuple",
 					} {
 						expectedError = expectedError || strings.Contains(es, exp)
 					}
diff --git a/pkg/sql/opt/optbuilder/testdata/union b/pkg/sql/opt/optbuilder/testdata/union
index 8da5e9163cd1..9625f180e023 100644
--- a/pkg/sql/opt/optbuilder/testdata/union
+++ b/pkg/sql/opt/optbuilder/testdata/union
@@ -1079,3 +1079,17 @@ sort
            ├── columns: t68702.a:7 b:8 c:9
            └── scan t68702
                 └── columns: t68702.a:7 b:8 c:9 rowid:10!null crdb_internal_mvcc_timestamp:11 tableoid:12
+
+
+# Regression test for #70831. Do not add non-existent casts to match types on
+# either side of a set operation.
+# NOTE: In Postgres this example successfully returns a result. Our tuple typing
+# and casting logic doesn't match Postgres's semantics exactly, so we return a
+# user error. See
+# https://github.com/cockroachdb/cockroach/issues/70831#issuecomment-929547297.
+build
+SELECT * FROM (VALUES (ARRAY[(true, NULL)])) AS v1
+EXCEPT ALL
+SELECT * FROM (VALUES (ARRAY[]::RECORD[])) AS v2
+----
+error (42804): EXCEPT types tuple{bool, unknown}[] and tuple[] cannot be matched
diff --git a/pkg/sql/opt/optbuilder/union.go b/pkg/sql/opt/optbuilder/union.go
index 25effe7a3db0..38d9b25aa8d9 100644
--- a/pkg/sql/opt/optbuilder/union.go
+++ b/pkg/sql/opt/optbuilder/union.go
@@ -153,15 +153,24 @@ func determineUnionType(left, right *types.T, clauseTag string) *types.T {
 	}
 
 	if left.Equivalent(right) {
-		// Do a best-effort attempt to determine which type is "larger".
-		if left.Width() > right.Width() {
-			return left
-		}
+		// In the default case, use the left type.
+		src, tgt := right, left
 		if left.Width() < right.Width() {
-			return right
+			// If the right type is "larger", use it.
+			src, tgt = left, right
 		}
-		// In other cases, use the left type.
-		return left
+		if !tree.ValidCast(src, tgt, tree.CastContextExplicit) {
+			// Error if no cast exists from src to tgt.
+			// TODO(#75103): For legacy reasons, we check for a valid cast in
+			// the most permissive context, CastContextExplicit. To be
+			// consistent with Postgres, we should check for a valid cast in the
+			// most restrictive context, CastContextImplicit.
+			panic(pgerror.Newf(
+				pgcode.DatatypeMismatch,
+				"%v types %s and %s cannot be matched", clauseTag, left, right,
+			))
+		}
+		return tgt
 	}
 	leftFam, rightFam := left.Family(), right.Family()
 
@@ -189,7 +198,7 @@ func determineUnionType(left, right *types.T, clauseTag string) *types.T {
 		return right
 	}
 
-	// TODO(radu): Postgres has more encompassing rules:
+	// TODO(#75103): Postgres has more encompassing rules:
 	// http://www.postgresql.org/docs/12/static/typeconv-union-case.html
 	panic(pgerror.Newf(
 		pgcode.DatatypeMismatch,
@@ -198,7 +207,9 @@ func determineUnionType(left, right *types.T, clauseTag string) *types.T {
 }
 
 // addCasts adds a projection to a scope, adding casts as necessary so that the
-// resulting columns have the given types.
+// resulting columns have the given types. This function assumes that there is a
+// valid cast from the column types in dst.cols to the corresponding types in
+// outTypes.
 func (b *Builder) addCasts(dst *scope, outTypes []*types.T) *scope {
 	expr := dst.expr
 	dstCols := dst.cols
diff --git a/pkg/sql/opt/optbuilder/union_test.go b/pkg/sql/opt/optbuilder/union_test.go
index 112d5159e3a0..557bd319b6bf 100644
--- a/pkg/sql/opt/optbuilder/union_test.go
+++ b/pkg/sql/opt/optbuilder/union_test.go
@@ -72,6 +72,12 @@ func TestUnionType(t *testing.T) {
 			right:    types.String,
 			expected: nil,
 		},
+		{
+			// Error.
+			left:     types.MakeArray(types.MakeTuple([]*types.T{types.Any})),
+			right:    types.MakeArray(types.MakeTuple([]*types.T{types.Bool})),
+			expected: nil,
+		},
 	}
 
 	for _, tc := range testCases {