Merge cockroachdb#52381

52381: sql: improve overload selection when multiple candidates require the same type r=RaduBerinde a=RaduBerinde

Our type checking cannot deal with expression `('{"x": "bar"}') -> 'x'`.
This works without the parens because literal constants are treated as
a special case. It used to work with parens because of some constant
folding that was done before type-checking (which was removed).

A principled fix would require big changes in type-checking. However,
I did notice that some of the logic for filtering overloads is too
restrictive. We only try to `TypeCheck` binary op arguments with a
specific type when we have a single candidate overload left. This is
why we don't try to type-check `('{"x": "bar"}')` as a Jsonb.

This change improves this heuristic: instead of checking whether we
have a single remaining candidate, check if all remaining candidates
require the same type for that argument. This is a net improvement
that happens to help in this particular case (where we have two
variants for `->`, both taking a Jsonb on the left).

Fixes cockroachdb#52043.

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

pkg/sql/opt/optbuilder/testdata/scalar

@@ -759,6 +759,20 @@ eq [type=bool]
       ├── const: 'bar' [type=string]
       └── const: 'baz' [type=string]
 
+build-scalar
+'{"x": "bar"}' -> 'x'
+----
+fetch-val [type=jsonb]
+ ├── const: '{"x": "bar"}' [type=jsonb]
+ └── const: 'x' [type=string]
+
+build-scalar
+('{"x": "bar"}') -> 'x'
+----
+fetch-val [type=jsonb]
+ ├── const: '{"x": "bar"}' [type=jsonb]
+ └── const: 'x' [type=string]
+
 build-scalar vars=(json)
 @1->>'a' = 'b'
 ----

pkg/sql/sem/tree/overload.go

@@ -516,10 +516,22 @@ func typeCheckOverloadedExprs(
 	// Filter out overloads on resolved types.
 	for _, i := range s.resolvableIdxs {
 		paramDesired := types.Any
-		if len(s.overloadIdxs) == 1 {
-			// Once we get down to a single overload candidate, begin desiring its
-			// parameter types for the corresponding argument expressions.
-			paramDesired = s.overloads[s.overloadIdxs[0]].params().GetAt(i)
+
+		// If all remaining candidates require the same type for this parameter,
+		// begin desiring that type for the corresponding argument expression.
+		// Note that this is always the case when we have a single overload left.
+		var sameType *types.T
+		for _, ovIdx := range s.overloadIdxs {
+			typ := s.overloads[ovIdx].params().GetAt(i)
+			if sameType == nil {
+				sameType = typ
+			} else if !typ.Identical(sameType) {
+				sameType = nil
+				break
+			}
+		}
+		if sameType != nil {
+			paramDesired = sameType
 		}
 		typ, err := exprs[i].TypeCheck(ctx, semaCtx, paramDesired)
 		if err != nil {

pkg/sql/sem/tree/type_check_test.go

@@ -176,6 +176,9 @@ func TestTypeCheck(t *testing.T) {
 		{`((ROW (1) AS a)).*`, `((1:::INT8,) AS a)`},
 		{`((('1'||'', 1+1) AS a, b)).*`, `(('1':::STRING || '':::STRING, 1:::INT8 + 1:::INT8) AS a, b)`},
 
+		{`'{"x": "bar"}' -> 'x'`, `'{"x": "bar"}':::JSONB->'x':::STRING`},
+		{`('{"x": "bar"}') -> 'x'`, `'{"x": "bar"}':::JSONB->'x':::STRING`},
+
 		// These outputs, while bizarre looking, are correct and expected. The
 		// type annotation is caused by the call to tree.Serialize, which formats the
 		// output using the Parseable formatter which inserts type annotations