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.

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

sql: concatenating null array incorrectly returns null #83091

Closed
rytaft opened this issue Jun 20, 2022 · 2 comments · Fixed by #84159
Closed

sql: concatenating null array incorrectly returns null #83091

rytaft opened this issue Jun 20, 2022 · 2 comments · Fixed by #84159
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team

Comments

@rytaft
Copy link
Collaborator

rytaft commented Jun 20, 2022

Concatenating a NULL array to any other array currently returns NULL in CockroachDB. However, array concatenation should actually just ignore NULL arguments. For example, this statement currently returns NULL in CockroachDB, which is incorrect:

SELECT ARRAY[]::BOOL[] || NULL::BOOL[];

It should return {}, which is what Postgres returns.

Jira issue: CRDB-16858

Epic: CRDB-14914

@rytaft rytaft added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jun 20, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Jun 20, 2022
@wenyihu6 wenyihu6 self-assigned this Jun 29, 2022
@rytaft rytaft changed the title opt: concatenating null array incorrectly returns null sql: concatenating null array incorrectly returns null Jun 29, 2022
@rytaft
Copy link
Collaborator Author

rytaft commented Jun 29, 2022

FYI, I looked at this issue and it's not an optimizer issue -- it's a difference between the vectorized engine and the row-based engine. Similar to how we check NullableArgs for comparison operations in colexec/colexeccmp/default_cmp_expr_tmpl.go, we'll also need to check NullableArgs for binary operations. As far as I can tell, the code in colexec/colexecproj/proj_non_const_ops_tmpl.go and colexec/colexecprojconst/proj_const_ops_tmpl.go is ignoring NullableArgs altogether. For example, this looks suspicious to me:

if !colNulls.NullAt(i) {
// We only want to perform the projection operation if the value is not null.

@rytaft
Copy link
Collaborator Author

rytaft commented Jun 29, 2022

Here's a reproduction that actually works (the repro in the issue description only works if constant folding is disabled in the optimizer):

[email protected]:26257/defaultdb> CREATE TABLE t (b BOOL[]);
CREATE TABLE


Time: 5ms total (execution 5ms / network 0ms)

[email protected]:26257/defaultdb> INSERT INTO t VALUES (ARRAY[]);
INSERT 0 1


Time: 6ms total (execution 6ms / network 0ms)

[email protected]:26257/defaultdb> SELECT b || NULL FROM t;
  ?column?
------------
  NULL
(1 row)

wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 11, 2022
Previously, the projection operators for binary or comparison expressions
directly copy nulls into the output without performing the actual projection
whenever either argument was null. This is incorrect because some function
operators can handle null arguments, and we want the actual projection result
instead. This PR fixed this by adding the boolean field nullableArgs to the
logic. Note that this pr is reviewable but not ready to merge yet.

TODO(wenyihu6): Add test cases.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in projection operators that gave output of
nulls when the expression can actually handle null arguments and may result in
a non-null output.
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 11, 2022
Previously, the projection operators for binary or comparison expressions
directly copy nulls into the output without performing the actual projection
whenever either argument was null. This is incorrect because some function
operators can handle null arguments, and we want the actual projection result
instead. This PR fixed this by adding the boolean field nullableArgs to the
logic. Note that this pr is reviewable but not ready to merge yet.

TODO(wenyihu6): Add test cases.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in projection operators that gave output of
nulls when the expression can actually handle null arguments and may result in
a non-null output.
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 13, 2022
Previously, the projection operators for binary or comparison expressions
directly copy nulls into the output without performing the actual projection
whenever either argument was null. This is incorrect because some function
operators can handle null arguments, and we want the actual projection result
instead. This PR fixed this by adding the boolean field nullableArgs to the
logic. Note that this pr is reviewable but not ready to merge yet.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in projection operators that gave output of
nulls when the expression can actually handle null arguments and may result in
a non-null output.
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 18, 2022
Previously, the projection operators for binary or comparison expressions
directly copy nulls into the output without performing the actual projection
whenever either argument was null. This is incorrect because some function
operators can handle null arguments, and we want the actual projection result
instead. Since currently the only projection operator that needs `nullableArgs
== true`
behaviour is `ConcatDatumDatum`, this PR fixed this by only adding a
boolean field `nullableArgs` to operators with `Datum`, `Datum`. If we later
introduce another projection operation that has nullableArgs == true, we should
update code accordingly.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in projection operators that gave output of
nulls when the expression can actually handle null arguments and may result in
a non-null output. Note that if we later
introduce another projection operation that has nullableArgs == true, we should
update code accordingly.
craig bot pushed a commit that referenced this issue Jul 18, 2022
82860: ui: Refactor Assertions to use Jest Expect r=nathanstilwell a=nathanstilwell

This is a draft pull request branched off of #82161 refactoring `assert` syntax (using [`chai`](https://www.npmjs.com/package/chai) and [`assert`](https://www.npmjs.com/package/assert)) with Jest's [`expect` syntax](https://jestjs.io/docs/expect). 

Since is based on top of [the Jest PR](#82161), this PR will remain in draft until this larger is merged and rebased

83712: cloud: support chaining of assumed roles r=rhu713 a=rhu713

Previously, a user could only assume a role in AWS or GCP directly, via the
identity specified by implicit or specified auth. This was inadequate because
there is a need to support role assumption through a chain of intermediate
delegate roles. To address this, this patch allows the ASSUME_ROLE parameter
in AWS and GCP storage and KMS URIs to specify a list of roles with a
comma-separated string. The roles in the list can then be chain assumed in
order to access the resource specified by the URI.

With this patch, if a destination in S3 can only be accessed by RoleB, and the
identity corresponding to implicit auth can only assume RoleB through an
intermediate role RoleA, then this chain assumption can be specified in the S3
URI:
s3://bucket/key?AUTH=implicit&ASSUME_ROLE=RoleA,RoleB

In addition, the parameters for auth in AWS URIs via assume role have been
changed so that the "assume" auth mode no longer exists, and the ASSUME_ROLE
param can be specified for both "specified" and "implicit" auth.

Finally, some AWS cloud unit tests, including the assume role tests, have been
added to the unit test nightly.

Release note (enterprise change): Allow the ASSUME_ROLE parameter in AWS and
GCP storage and KMS URIs to specify a list of roles with a comma-separated
string. The roles in the list can then be chain assumed in order to access the
resource specified by the URI.

For example, if a destination in S3 can only be accessed by RoleB, and the
identity corresponding to implicit auth can only assume RoleB through an
intermediate role RoleA, then this chain assumption can be specified in the S3
URI:
s3://bucket/key?AUTH=implicit&ASSUME_ROLE=RoleA,RoleB

In addition, remove the "assume" auth mode from AWS URIs, and instead use the
ASSUME_ROLE parameter to indicate that a role should be assumed for
authentication. Below are some examples:

S3: s3://<bucket>/<key>?AUTH=specified&ASSUME_ROLE=<role_arn>&AWS_ACCESS_KEY_ID=<access_key>&AWS_SECRET_ACCESS_KEY=<secret_key>
AWS KMS: aws:///<key_arn>?AUTH=implicit&REGION=<region>&ASSUME_ROLE=<role_arn>

83868: tree: fix vectorized COALESCE and NULLIF type checking with VOID r=msirek a=msirek

Fixes #83754

Previously, the COALESCE and NULLIF operators could error out in
vectorized execution when comparing a VOID datum with NULL.

This occurred due to the unique property of a VOID, that it
can't be compared with another VOID using any of the normal operators
such as `=`, `<`, `>`..., or even IS [NOT] [DISTINCT FROM], for example:
```
SELECT ''::VOID IS DISTINCT FROM ''::VOID;
ERROR: unsupported comparison operator: <void> IS DISTINCT FROM <void>
SQLSTATE: 22023
```
Processing of COALESCE in the columnar execution engine for an
expression like `COALESCE(void_col, NULL)` builds an IS DISTINCT FROM
operation between the VOID column and NULL here:
https://github.com/cockroachdb/cockroach/blob/ea559dfe0ba57259ca71d3c8ca1de6388954ea73/pkg/sql/colexec/colbuilder/execplan.go#L2122-L2129

This comparison is assumed to be OK, but fails with an internal error
because comparison with `UnknownType` is only implicitly supported if
the type can be compared with itself.

To address this, this patch modifies vectorized COALESCE to use the
IS NOT NULL operator internally instead of IS DISTINCT FROM whenever    
the latter is not a valid comparison.
A similar problem with NULLIF, which internally uses `=`, is fixed.
Type checking of NULLIF is enhanced in the parser so incompatible
comparisons can be caught prior to execution time.

Release note (bug fix): This patch fixes vectorized evaluation of
COALESCE involving expressions of type VOID and enhances type checking
of NULLIF expressions with VOID, so incompatible comparisons can
be caught during query compilation instead of during query execution.

84159: colexec: fixed nullableArgs projection operators  r=wenyihu6 a=wenyihu6

Previously, the `Concat` projection operators on arrays directly copy nulls into
the output without performing the actual projection whenever either argument was
null. This is incorrect because `Concat` operator can actually handle null arguments,
and we want the actual projection result instead. Since currently the only
projection operator that needs `nullableArgs == true` behaviour is
`ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs`
to the logic for operators with `Datum`, `Datum`. If we later introduce another
projection operation that has nullableArgs == true, we should update this code
accordingly.

Fixes: #83091

Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays
that gave output of nulls when the projection operator can actually handle null
arguments and may result in a non-null output.

Co-authored-by: Nathan Stilwell <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Rui Hu <[email protected]>
Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: wenyihu3 <[email protected]>
@craig craig bot closed this as completed in f07a903 Jul 18, 2022
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 18, 2022
Previously, the `Concat` projection operators on arrays directly copy nulls into
the output without performing the actual projection whenever either argument was
null. This is incorrect because `Concat` operator can actually handle null arguments,
and we want the actual projection result instead. Since currently the only
projection operator that needs `nullableArgs == true` behaviour is
`ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs`
to the logic for operators with `Datum`, `Datum`. If we later introduce another
projection operation that has nullableArgs == true, we should update this code
accordingly.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays
that gave output of nulls when the projection operator can actually handle null
arguments and may result in a non-null output.
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 23, 2022
Previously, the `Concat` projection operators on arrays directly copy nulls into
the output without performing the actual projection whenever either argument was
null. This is incorrect because `Concat` operator can actually handle null arguments,
and we want the actual projection result instead. Since currently the only
projection operator that needs `nullableArgs == true` behaviour is
`ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs`
to the logic for operators with `Datum`, `Datum`. If we later introduce another
projection operation that has nullableArgs == true, we should update this code
accordingly.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays
that gave output of nulls when the projection operator can actually handle null
arguments and may result in a non-null output.
blathers-crl bot pushed a commit that referenced this issue Jul 24, 2022
Previously, the `Concat` projection operators on arrays directly copy nulls into
the output without performing the actual projection whenever either argument was
null. This is incorrect because `Concat` operator can actually handle null arguments,
and we want the actual projection result instead. Since currently the only
projection operator that needs `nullableArgs == true` behaviour is
`ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs`
to the logic for operators with `Datum`, `Datum`. If we later introduce another
projection operation that has nullableArgs == true, we should update this code
accordingly.

Fixes: #83091

Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays
that gave output of nulls when the projection operator can actually handle null
arguments and may result in a non-null output.
wenyihu6 added a commit to wenyihu6/cockroach that referenced this issue Jul 24, 2022
Previously, the `Concat` projection operators on arrays directly copy nulls into
the output without performing the actual projection whenever either argument was
null. This is incorrect because `Concat` operator can actually handle null arguments,
and we want the actual projection result instead. Since currently the only
projection operator that needs `nullableArgs == true` behaviour is
`ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs`
to the logic for operators with `Datum`, `Datum`. If we later introduce another
projection operation that has nullableArgs == true, we should update this code
accordingly.

Fixes: cockroachdb#83091

Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays
that gave output of nulls when the projection operator can actually handle null
arguments and may result in a non-null output.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants