Merge cockroachdb#46649

46649: sql: fix type checking code for aggregate functions r=rytaft a=rytaft

Prior to this commit, any aggregate function that had an argument with
unknown type was replaced with `NULL`. This is incorrect for scalar
aggregates when the input relation has multiple rows, because after
replacement, the query result has the same number of rows as the input
relation. It should instead be reduced to a single row.

This commit fixes the issue by avoiding replacing the aggregate with `NULL`.

Note that for many aggregates, this change results in an ambiguous function
error since the type checking code cannot choose which overload is correct
for the unknown type. This is different behavior than Postgres, which
defaults to type "text".

Fixes cockroachdb#46196

Release justification: this is a low risk, high benefit change to existing
functionality.
Release note (bug fix): fixed an incorrect query result that could occur
when a scalar aggregate was called with a null input.

Co-authored-by: Rebecca Taft <[email protected]>

pkg/sql/logictest/testdata/logic_test/typing

@@ -221,3 +221,14 @@ query I
 SELECT NULLIF(0, NULL) + NULLIF(0, NULL)
 ----
 0
+
+# Regression test for #46196.
+query T
+SELECT max(t0.c0) FROM (VALUES (NULL), (NULL)) t0(c0)
+----
+NULL
+
+query T
+SELECT max(NULL) FROM (VALUES (NULL), (NULL)) t0(c0)
+----
+NULL

pkg/sql/opt/optbuilder/testdata/aggregate

@@ -2706,12 +2706,9 @@ scalar-group-by
 build
 SELECT corr(k, NULL) FROM kv
 ----
-project
- ├── columns: corr:5
- ├── scan kv
- │    └── columns: k:1!null v:2 w:3 s:4
- └── projections
-      └── NULL [as=corr:5]
+error (42725): ambiguous call: corr(int, unknown), candidates are:
+corr(int, int) -> float
+corr(int, float) -> float
 
 build
 SELECT corr('foo', v) FROM kv
@@ -3839,3 +3836,22 @@ project
                      │              └── b:3
                      └── projections
                           └── (max:4, unnest:1) [as="?column?":6]
+
+# Regression test for #46196. Don't eliminate the scalar group by, and
+# default to type string.
+build format=show-types
+SELECT max(t0.c0) FROM (VALUES (NULL), (NULL)) t0(c0);
+----
+scalar-group-by
+ ├── columns: max:3(string)
+ ├── project
+ │    ├── columns: column2:2(string)
+ │    ├── values
+ │    │    ├── columns: column1:1(unknown)
+ │    │    ├── (NULL,) [type=tuple{unknown}]
+ │    │    └── (NULL,) [type=tuple{unknown}]
+ │    └── projections
+ │         └── column1:1::STRING [as=column2:2, type=string]
+ └── aggregations
+      └── max [as=max:3, type=string]
+           └── column2:2 [type=string]

pkg/sql/sem/tree/type_check.go

@@ -847,10 +847,46 @@ func (expr *FuncExpr) TypeCheck(ctx *SemaContext, desired *types.T) (TypedExpr,
 			"%s()", def.Name)
 	}
 
+	// If the function is an aggregate that does not accept null arguments and we
+	// have arguments of unknown type, see if we can assign type string instead.
+	// TODO(rytaft): If there are no overloads with string inputs, Postgres
+	// chooses the overload with preferred type for the given category. For
+	// example, float8 is the preferred type for the numeric category in Postgres.
+	// To match Postgres' behavior, we should add that logic here too.
+	if !def.NullableArgs && def.FunctionProperties.Class == AggregateClass {
+		for i := range typedSubExprs {
+			if typedSubExprs[i].ResolvedType().Family() == types.UnknownFamily {
+				var filtered []overloadImpl
+				for j := range fns {
+					if fns[j].params().GetAt(i).Equivalent(types.String) {
+						if filtered == nil {
+							filtered = make([]overloadImpl, 0, len(fns)-j)
+						}
+						filtered = append(filtered, fns[j])
+					}
+				}
+
+				// Only use the filtered list if it's not empty.
+				if filtered != nil {
+					fns = filtered
+
+					// Cast the expression to a string so the execution engine will find
+					// the correct overload.
+					e, err := NewTypedCastExpr(typedSubExprs[i], types.String)
+					if err != nil {
+						return nil, err
+					}
+					typedSubExprs[i] = e
+				}
+			}
+		}
+	}
+
 	// Return NULL if at least one overload is possible, no overload accepts
-	// NULL arguments, the function isn't a generator builtin, and NULL is given
-	// as an argument.
-	if !def.NullableArgs && def.FunctionProperties.Class != GeneratorClass {
+	// NULL arguments, the function isn't a generator or aggregate builtin, and
+	// NULL is given as an argument.
+	if !def.NullableArgs && def.FunctionProperties.Class != GeneratorClass &&
+		def.FunctionProperties.Class != AggregateClass {
 		for _, expr := range typedSubExprs {
 			if expr.ResolvedType().Family() == types.UnknownFamily {
 				return DNull, nil