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: fix ColumnAccessExpr.Eval with NULL inner expression #78423

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Mar 24, 2022

sql: fix ColumnAccessExpr.Eval with NULL inner expression

Previously, ColumnAccessExpr.Eval would panic if the
ColumnAccessExpr's inner expression evaluated to NULL, because it
attempted to cast this NULL to a DTuple. Now, if the inner
expression is NULL, ColumnAccessExpr.Eval returns NULL.

Fixes #78159

Release note (bug fix): A bug has been fixed that caused an internal
error when the inner expression of a column access expression evaluated
to NULL. For example, evaluation of the expression
(CASE WHEN b THEN ((ROW(1) AS a)) ELSE NULL END).a would error when
b is false. This bug has been present since version 19.1 or earlier.

sql: include tuple labels in types returned from typeCheckSameTypedTupleExprs

Previously, an expression that produced a tuple from several potential
values was typed as a tuple without any labels. This prevented a tuple's
column to be accessed by a label name.

For example, the expression below would result in the error "could not
identify column 'a' in record data type".

SELECT (CASE WHEN true THEN (ROW(1) AS a) ELSE (ROW(2) AS a) END).a

Now, the labels of the first tuple are used in the type of the outer
expression.

Fixes #78515

Release note (bug fix): A bug has been fixed that caused an error when
accessing a named column of a labelled tuple. The bug only occurred when
an expression could produce one of several different tuples. For
example, (CASE WHEN true THEN (ROW(1) AS a) ELSE (ROW(2) AS a) END).a
would fail to evaluate. This bug was present since version 22.1.0.
Although present in previous versions, it was impossible to encounter
due to limitations that prevented using tuples in this way.

@mgartner mgartner requested review from msirek, cucaroach and a team March 24, 2022 14:35
@mgartner mgartner requested a review from a team as a code owner March 24, 2022 14:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix! Nice job reverse-engineering the test case.
:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

Copy link
Contributor

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM:

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained

@mgartner
Copy link
Collaborator Author

The test case from the first commit revealed another bug, #75101. I've added another commit to address it, so please take another look @msirek @cucaroach.

Previously, `ColumnAccessExpr.Eval` would panic if the
`ColumnAccessExpr`'s inner expression evaluated to `NULL`, because it
attempted to cast this `NULL` to a `DTuple`. Now, if the inner
expression is `NULL`, `ColumnAccessExpr.Eval` returns `NULL`.

Fixes cockroachdb#78159

Release note (bug fix): A bug has been fixed that caused an internal
error when the inner expression of a column access expression evaluated
to `NULL`. For example, evaluation of the expression
`(CASE WHEN b THEN ((ROW(1) AS a)) ELSE NULL END).a` would error when
`b` is `false`. This bug has been present since version 19.1 or earlier.
…pleExprs

Previously, an expression that produced a tuple from several potential
values was typed as a tuple without any labels. This prevented a tuple's
column to be accessed by a label name.

For example, the expression below would result in the error "could not
identify column 'a' in record data type".

    SELECT (CASE WHEN true THEN (ROW(1) AS a) ELSE (ROW(2) AS a) END).a

Now, the labels of the first tuple are used in the type of the outer
expression.

Fixes cockroachdb#78515

Release note (bug fix): A bug has been fixed that caused an error when
accessing a named column of a labelled tuple. The bug only occurred when
an expression could produce one of several different tuples. For
example, `(CASE WHEN true THEN (ROW(1) AS a) ELSE (ROW(2) AS a) END).a`
would fail to evaluate. This bug was present since version 22.1.0.
Although present in previous versions, it was impossible to encounter
due to limitations that prevented using tuples in this way.
@mgartner
Copy link
Collaborator Author

Friendly ping to take a look at the latest commit. @msirek @cucaroach Thanks!

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 5, 2022

Friendly ping 😃

Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest code revision looks good. I tried out some test cases and all cases enforce that all THEN and ELSE branches conform to the type of the first THEN branch.
:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 6, 2022

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 6, 2022

Build succeeded:

@cucaroach
Copy link
Contributor

cucaroach commented Apr 6, 2022

I guess once its merged reviewable goes into retirement? Anyways :lgtm:

@mgartner mgartner deleted the 78159-column-access-fix branch April 6, 2022 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants