Merge #148863

148863: optbuilder: prohibit some operations on REFCURSOR r=yuzefovich a=yuzefovich

Postgres doesn't allow ordering by and equality comparison on refcursor columns, so this commit adds checks to match that behavior.

Fixes: #145477.

Release note (bug fix): CockroachDB now prohibits ORDER BY and join equality operations on REFCURSOR type, matching behavior of Postgres.

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/refcursor

@@ -470,6 +470,12 @@ SELECT NULLIF('foo'::REFCURSOR, 'bar'::REFCURSOR);
 statement error pgcode 42883 pq: unsupported comparison operator
 SELECT NULLIF('foo'::REFCURSOR, NULL);
 
+statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
+SELECT * FROM t ORDER BY x;
+
+statement error pgcode 42883 pq: operator does not exist: refcursor = refcursor
+SELECT * FROM t INNER JOIN t t2 USING (x);
+
 # Regression test for #112010 - REFCURSOR should allow IS NULL and
 # IS NOT NULL comparisons.
 subtest is_null
@@ -685,22 +691,35 @@ baz   NULL  baz  foo
 statement error pgcode 42883 pq: unknown signature
 SELECT lag(y) OVER w, lead(y) OVER w, first_value(y) OVER w, last_value(y) OVER w FROM t WINDOW w AS ();
 
-# Array functions are allowed.
+# Array functions are allowed (but ordering on REFCURSOR and REFCURSOR[] columns
+# is not).
 query TT
-SELECT array_agg(x ORDER BY x, y), array_cat_agg(y ORDER BY x, y) FROM t;
+SELECT array_agg(x), array_cat_agg(y) FROM t;
 ----
-{baz,foo}  {bar}
+{foo,baz}  {bar}
 
-query TT
-SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY x, y;
+query TT rowsort
+SELECT array_append(y, x), array_prepend(x, y) FROM t;
 ----
 {baz}      {baz}
 {bar,foo}  {foo,bar}
 
-query TT
-SELECT array_remove(y, 'baz'), array_replace(y, 'baz', 'foo') FROM t ORDER BY x, y;
+query TT rowsort
+SELECT array_remove(y, 'baz'), array_replace(y, 'baz', 'foo') FROM t;
 ----
 {}     {}
 {bar}  {bar}
 
+statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
+SELECT array_agg(x ORDER BY x) FROM t;
+
+statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor\[\]
+SELECT array_agg(y ORDER BY y) FROM t;
+
+statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
+SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY x;
+
+statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor\[\]
+SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY y;
+
 subtest end

pkg/sql/opt/optbuilder/groupby.go

@@ -730,6 +730,7 @@ func (b *Builder) buildAggregateFunction(
 					expr := tree.NewTypedIsNullExpr(e)
 					b.buildAggArg(expr, &info, tempScope, fromScope)
 				}
+				ensureColumnOrderable(e)
 				b.buildAggArg(e, &info, tempScope, fromScope)
 			}
 		}

pkg/sql/opt/optbuilder/join.go

@@ -482,6 +482,11 @@ func (jb *usingJoinBuilder) addEqualityCondition(leftCol, rightCol *scopeColumn)
 				"JOIN/USING types %s for left and %s for right cannot be matched for column %q",
 				leftCol.typ, rightCol.typ, tree.ErrString(&name)))
 		}
+	} else if !tree.EqCmpAllowedForEquivalentTypes(leftCol.typ, rightCol.typ) {
+		panic(pgerror.Newf(pgcode.UndefinedFunction,
+			"operator does not exist: %s = %s",
+			leftCol.typ.SQLStandardName(), rightCol.typ.SQLStandardName(),
+		))
 	}
 
 	// We will create a new "merged" column and hide the original columns.

pkg/sql/opt/optbuilder/orderby.go

@@ -325,14 +325,15 @@ func (b *Builder) hasDefaultNullsOrder(order *tree.Order) bool {
 
 func ensureColumnOrderable(e tree.TypedExpr) {
 	typ := e.ResolvedType()
+	var arraySuffix string
 	if typ.Family() == types.ArrayFamily {
 		typ = typ.ArrayContents()
+		arraySuffix = "[]"
 	}
 	switch typ.Family() {
 	case types.TSQueryFamily, types.TSVectorFamily, types.PGVectorFamily:
 		panic(unimplementedWithIssueDetailf(92165, "", "can't order by column type %s", typ.SQLString()))
-	case types.JsonpathFamily:
-		panic(pgerror.Newf(pgcode.UndefinedFunction, "could not identify an ordering operator for type jsonpath"))
-
+	case types.RefCursorFamily, types.JsonpathFamily:
+		panic(pgerror.Newf(pgcode.UndefinedFunction, "could not identify an ordering operator for type %s%s", typ.SQLStandardName(), arraySuffix))
 	}
 }

pkg/sql/sem/tree/eval.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/errors"
@@ -2170,3 +2171,19 @@ func EqualComparisonFunctionExists(leftType, rightType *types.T) bool {
 	_, found := CmpOps[treecmp.EQ].LookupImpl(leftType, rightType)
 	return found
 }
+
+// EqCmpAllowedForEquivalentTypes returns whether an equality comparison is
+// allowed between two equivalent types.
+func EqCmpAllowedForEquivalentTypes(leftType, rightType *types.T) bool {
+	if buildutil.CrdbTestBuild {
+		if !leftType.Equivalent(rightType) {
+			panic(errors.AssertionFailedf(
+				"expected equivalent types, found %s, %s",
+				leftType.SQLStringForError(), rightType.SQLStringForError(),
+			))
+		}
+	}
+	err := runValidations(treecmp.EQ, leftType, rightType,
+		[]types.Family{types.RefCursorFamily, types.JsonpathFamily})
+	return err == nil
+}

pkg/sql/sem/tree/type_check.go

@@ -3782,6 +3782,8 @@ var CannotAcceptTriggerErr = pgerror.New(pgcode.FeatureNotSupported,
 // given family, which is invalid for comparison. We don't simply remove
 // the relevant comparison overloads because we rely on their existence in
 // various locations throughout the codebase.
+// TODO(yuzefovich): audit callers of this method to see whether Jsonpath family
+// should be handled in the same way as RefCursor family is.
 func checkComparison(
 	op treecmp.ComparisonOperatorSymbol, left, right *types.T, family types.Family,
 ) error {