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

tree: handle null in suboperator expressions #37775

Merged
merged 1 commit into from May 24, 2019

Conversation

Projects
None yet
4 participants
@rafiss
Copy link
Collaborator

commented May 23, 2019

Before this change, a null right operand in a suboperator would cause an
unhandled error. We were receiving stack trace reports from the wild.
Add a test and fix it.

fixes #37547
Release note (bug fix): a null right operand now causes the suboperator
expression to return null

@rafiss rafiss requested review from jordanlewis and asubiotto May 23, 2019

@rafiss rafiss requested a review from cockroachdb/sql-rest-prs as a code owner May 23, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 23, 2019

This change is Reviewable

@rafiss rafiss added the first-pr label May 23, 2019

@jordanlewis
Copy link
Member

left a comment

This :lgtm: - nice! Thinking again about it, I think you could solve the problem and also clean up the previous code a tiny bit at the same time with a small change. I left an inline suggestion.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @rafiss)


pkg/sql/sem/tree/eval.go, line 3686 at r1 (raw file):

	_, newLeft, newRight, _, not := foldComparisonExpr(op, left, right)
	if !expr.fn.NullableArgs && (newLeft == DNull || newRight == DNull) {

I just realized that this code should also apply to the case with suboperators, so I think we could pull this check up above the suboperator check and avoid the new special case.

This should be valid because NULL = ANY (...) is always NULL, and x = ANY NULL is always NULL as well.

(and the detail about running this on newLeft and newRight isn't important - if you click into foldComparisonExpr, that might just reverse the order of left and right - but since we're checking if either of them for NULL-ness, we can just as well run the check before folding as after.)


pkg/sql/sem/tree/testdata/eval/any_some_all, line 74 at r1 (raw file):


eval
1 = ANY(NULL::int[])

small style thing: For test cases like this one, (e.g. it might not be super obvious why you need the extra cast), we typically add a comment above the test case that says that it's a regression test for an issue. So you'd say something approximately like "# Regression test for #issuenum - ensure that null RHS of comparions with suboperators are correctly handled."

@rafiss
Copy link
Collaborator Author

left a comment

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


pkg/sql/sem/tree/eval.go, line 3686 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I just realized that this code should also apply to the case with suboperators, so I think we could pull this check up above the suboperator check and avoid the new special case.

This should be valid because NULL = ANY (...) is always NULL, and x = ANY NULL is always NULL as well.

(and the detail about running this on newLeft and newRight isn't important - if you click into foldComparisonExpr, that might just reverse the order of left and right - but since we're checking if either of them for NULL-ness, we can just as well run the check before folding as after.)

Done.


pkg/sql/sem/tree/testdata/eval/any_some_all, line 74 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

small style thing: For test cases like this one, (e.g. it might not be super obvious why you need the extra cast), we typically add a comment above the test case that says that it's a regression test for an issue. So you'd say something approximately like "# Regression test for #issuenum - ensure that null RHS of comparions with suboperators are correctly handled."

Done.

@rafiss rafiss force-pushed the rafiss:suboperator-null-handling branch from 62a8e63 to 66edbc4 May 23, 2019

@jordanlewis
Copy link
Member

left a comment

:lgtm_strong:

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

@asubiotto
Copy link
Contributor

left a comment

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@rafiss rafiss force-pushed the rafiss:suboperator-null-handling branch from 66edbc4 to a317980 May 23, 2019

@asubiotto

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Looks like CI is failing. TeamCity is pretty confusing so let me know if you want a hand in figuring out what's wrong.

tree: handle null in suboperator expressions
Before this change, a null right operand in a suboperator would cause an
unhandled error. We were receiving stack trace reports from the wild.
Add a test and fix it. Also add a few additional tests to verify correct
behavior with a null left operand.

Release note (bug fix): a null right operand now causes the suboperator
expression to return null

@rafiss rafiss force-pushed the rafiss:suboperator-null-handling branch from a317980 to d610375 May 24, 2019

@rafiss
Copy link
Collaborator Author

left a comment

turns out the earlier suggestion to move the check earlier in the code breaks other behavior -- a null LHS in suboperators has different behavior. i went back to the original version and added more tests.

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

@rafiss

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 24, 2019

Merge #37775
37775: tree: handle null in suboperator expressions r=rafiss a=rafiss

Before this change, a null right operand in a suboperator would cause an
unhandled error. We were receiving stack trace reports from the wild.
Add a test and fix it.

fixes #37547 
Release note (bug fix): a null right operand now causes the suboperator
expression to return null

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

commented May 24, 2019

Build succeeded

@craig craig bot merged commit d610375 into cockroachdb:master May 24, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@jordanlewis

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Woo!

Since this is an isolated bugfix for a correctness issue that people have been seeing in the wild, you should also backport it to the 19.1 release branch so that it will be available in the next 19.1 patch release. There are instructions for this on the wiki.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.