Skip to content

Commit

Permalink
sql: fix type checking code for aggregate functions
Browse files Browse the repository at this point in the history
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 #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.
  • Loading branch information
rytaft committed Mar 26, 2020
1 parent b5abb6e commit 6ecec23
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 3 deletions.
7 changes: 7 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/typing
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,10 @@ query I
SELECT NULLIF(0, NULL) + NULLIF(0, NULL)
----
0

# Regression test for #46196.
query error ambiguous call: max\(unknown\)
SELECT MAX(t0.c0) FROM (VALUES (NULL), (NULL)) t0(c0)

query error ambiguous call: max\(unknown\)
SELECT MAX(NULL) FROM (VALUES (NULL), (NULL)) t0(c0)
23 changes: 23 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/aggregate
Original file line number Diff line number Diff line change
Expand Up @@ -3839,3 +3839,26 @@ project
│ └── b:3
└── projections
└── (max:4, unnest:1) [as="?column?":6]

# Regression test for #46196.
build
SELECT MAX(t0.c0) FROM (VALUES (NULL), (NULL)) t0(c0);
----
error (42725): ambiguous call: max(unknown), candidates are:
max(int) -> int
max(float) -> float
max(decimal) -> decimal
max(date) -> date
max(timestamp) -> timestamp
max(interval) -> interval
max(string) -> string
max(bytes) -> bytes
max(timestamptz) -> timestamptz
max(oid) -> oid
max(uuid) -> uuid
max(inet) -> inet
max(time) -> time
max(timetz) -> timetz
max(jsonb) -> jsonb
max(varbit) -> varbit
max(bool) -> bool
7 changes: 4 additions & 3 deletions pkg/sql/sem/tree/type_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,9 +848,10 @@ func (expr *FuncExpr) TypeCheck(ctx *SemaContext, desired *types.T) (TypedExpr,
}

// 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
Expand Down

0 comments on commit 6ecec23

Please sign in to comment.