Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VALUES table expression malfunctions with MAX #46196

Closed
mrigger opened this issue Mar 17, 2020 · 1 comment · Fixed by #46649
Closed

VALUES table expression malfunctions with MAX #46196

mrigger opened this issue Mar 17, 2020 · 1 comment · Fixed by #46649
Assignees
@rytaft
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@mrigger
Copy link

mrigger commented Mar 17, 2020

Consider the following test case:

SELECT MAX(t0.c0) FROM (VALUES (NULL), (NULL)) t0(c0); -- expected: {NULL}, actual: {NULL, NULL}

Unexpectedly, the query returns two rows, rather than one. This bug seems to occur only when a VALUES expression contains NULL values only.
@mrigger mrigger changed the title VALUES table expression malfunctions with MAX. VALUES table expression malfunctions with MAX Mar 17, 2020
@yuzefovich yuzefovich added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Mar 17, 2020
@andy-kimball
Copy link
Contributor

Looking at optsteps, I think this may be an optbuilder agg issue.

rytaft added a commit to rytaft/cockroach that referenced this issue Mar 26, 2020
@rytaft
sql: fix type checking code for aggregate functions
6ecec23 
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.
@rytaft rytaft mentioned this issue Mar 26, 2020
Merged
craig bot pushed a commit that referenced this issue Mar 30, 2020
@rytaft
Merge #46649
93cb2eb 
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 #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 <becca@cockroachlabs.com>
@craig craig bot closed this as completed in 64c2fe7 Mar 30, 2020
rytaft added a commit to rytaft/cockroach that referenced this issue Mar 31, 2020
@rytaft
sql: fix type checking code for aggregate functions
cba807d 
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.
@rytaft rytaft mentioned this issue Mar 31, 2020
Merged
rytaft added a commit to rytaft/cockroach that referenced this issue Apr 1, 2020
@rytaft
sql: fix type checking code for aggregate functions
7b0456d 
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.
@rytaft rytaft mentioned this issue Apr 1, 2020
Merged
rytaft added a commit to rytaft/cockroach that referenced this issue Apr 1, 2020
@rytaft
sql: fix type checking code for aggregate functions
ce22fbe 
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.
@rytaft rytaft mentioned this issue Apr 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rytaft rytaft

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: fix type checking code for aggregate functions rytaft/cockroach
4 participants
@rytaft @yuzefovich @mrigger @andy-kimball