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

distsql: planning error related to typed NULLs #25724

Closed
RaduBerinde opened this issue May 19, 2018 · 1 comment · Fixed by #25735
Closed

distsql: planning error related to typed NULLs #25724

RaduBerinde opened this issue May 19, 2018 · 1 comment · Fixed by #25735
Assignees
@RaduBerinde
Labels
A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@RaduBerinde
Copy link
Member 

SELECT ARRAY_AGG(NULL::TEXT) FROM (VALUES (1)) AS t;
pq: no builtin aggregate for ARRAY_AGG on [{NULL 0 0 [] <nil> NONE <nil>}]

The problem is that we are normalizing NULL::TEXT to DNull which has the null type, not the text type. This is also a problem with NULLs built by the optimizer.

One option is to keep the CastExpr. Another fix for this particular case is to add an ARRAY_AGG version for the null type; I am not sure what other cases there are (where the type of a null const matters).

CC @knz @justinj
@RaduBerinde RaduBerinde self-assigned this May 19, 2018
@RaduBerinde
Copy link
Member Author

I think the sound thing to do is for expression normalization never to return a TypedExpr with a different ResolvedType() than the original, even if that means adding a cast to DNull.

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue May 20, 2018
@RaduBerinde
sql: fix null normalization
20c77df 
The normalization rules are happy to convert `NULL::TEXT` to `NULL`.
While both expressions evaluate to `DNull`, the `ResolvedType()` is
different. It seems unsound for normalization to change the type.

This issue is shown by trying to run a query containing
`ARRAY_AGG(NULL::TEXT)` through distsql planning: by the time the
distsql planner looks at it, the `NULL::TEXT` is just `DNull` (with
the `Unknown` type) and the distsql planner cannot find the builtin.

This change fixes the normalization rules by retaining the cast in
this case. In general, any expression that statically evaluates to
NULL gets a cast to the original expression type. The same is done in
the opt execbuilder.

Fixes cockroachdb#25724.

Release note (bug fix): Fixed query errors in some cases involving a
NULL constant that is cast to a specific type.
@RaduBerinde RaduBerinde mentioned this issue May 20, 2018
Merged
@RaduBerinde RaduBerinde added A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels May 20, 2018
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue May 20, 2018
@RaduBerinde
sql: fix null normalization
9ba2fd7 
The normalization rules are happy to convert `NULL::TEXT` to `NULL`.
While both expressions evaluate to `DNull`, the `ResolvedType()` is
different. It seems unsound for normalization to change the type.

This issue is shown by trying to run a query containing
`ARRAY_AGG(NULL::TEXT)` through distsql planning: by the time the
distsql planner looks at it, the `NULL::TEXT` is just `DNull` (with
the `Unknown` type) and the distsql planner cannot find the builtin.

This change fixes the normalization rules by retaining the cast in
this case. In general, any expression that statically evaluates to
NULL gets a cast to the original expression type. The same is done in
the opt execbuilder.

Fixes cockroachdb#25724.

Release note (bug fix): Fixed query errors in some cases involving a
NULL constant that is cast to a specific type.
craig bot pushed a commit that referenced this issue May 21, 2018
@bdarnell @RaduBerinde
Merge #24956 #25735
d865309 
24956: storage: Maintain a separate set of unquiesced replicas r=petermattis a=bdarnell

This means that idle replicas no longer have a per-tick CPU cost,
which is one of the bottlenecks limiting the amount of data we can
handle per store.

Fixes #17609

Release note (performance improvement): Reduced CPU overhead of idle
ranges


The first five commits are from #24920; that PR should be merged and tested in isolation first. 

25735: sql: fix null normalization r=RaduBerinde a=RaduBerinde

The normalization rules are happy to convert `NULL::TEXT` to `NULL`.
While both expressions evaluate to `DNull`, the `ResolvedType()` is
different. It seems unsound for normalization to change the type.

This issue is shown by trying to run a query containing
`ARRAY_AGG(NULL::TEXT)` through distsql planning: by the time the
distsql planner looks at it, the `NULL::TEXT` is just `DNull` (with
the `Unknown` type) and the distsql planner cannot find the builtin.

This change fixes the normalization rules by retaining the cast in
this case. In general, any expression that statically evaluates to
NULL gets a cast to the original expression type. The same is done in
the opt execbuilder.

Fixes #25724.

Release note (bug fix): Fixed query errors in some cases involving a
NULL constant that is cast to a specific type.

Co-authored-by: Ben Darnell <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in #25735 May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaduBerinde RaduBerinde

Labels
A-sql-optimizer SQL logical planning and optimizations. 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 null normalization RaduBerinde/cockroach
1 participant
@RaduBerinde