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: allow refcursor comparison with NULL values #112070

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

DrewKimball
Copy link
Collaborator

Most comparisons are disallowed for the REFCURSOR data type. For example, the following is not allowed in postgres:

SELECT 'foo'::REFCURSOR = 'bar'::REFCURSOR;

However, postgres does allow using IS [NOT] DISTINCT FROM NULL and IS [NOT] NULL with a refcursor-typed expression os the left operand. There are a few places where CRDB internally expects this comparison to be valid as well, so disallowing these comparisons has caused test failures.

This patch removes the refcursor-specific check for invalid comparisons during type checking, and relies on execution-time checks as is done for other comparisons. This means that IsNull and IsNotNull expressions can now be executed. In addition, the IS DISTINCT FROM NULL comparison is now supported only for the case of type REFCURSOR as the left operand, and UNKNOWN (NULL) as the right operand.

Fixes #112010
Fixes #112011

Release note: None

@DrewKimball DrewKimball requested review from mgartner, rharding6373 and a team October 9, 2023 21:01
@DrewKimball DrewKimball requested a review from a team as a code owner October 9, 2023 21:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rharding6373 rharding6373 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! 1 of 0 LGTMs obtained (waiting on @mgartner)

@DrewKimball DrewKimball force-pushed the refcursor-comparison branch from ce698e1 to 753606d Compare October 9, 2023 21:14
Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm: with 1 suggestion

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball)


-- commits line 20 at r2:
Very interesting behavior! Good job getting to the bottom of it!


pkg/sql/logictest/testdata/logic_test/refcursor line 514 at r2 (raw file):

true
true
true

Can you add some tests for values on the RHS that lead to errors? From my testing, anything other than an untyped NULL will cause operator does not exist errors: column refs, CASE expressions, typed NULLs, etc.

Most comparisons are disallowed for the REFCURSOR data type. For example,
the following is not allowed in postgres:
```
SELECT 'foo'::REFCURSOR = 'bar'::REFCURSOR;
```
However, postgres does allow using `IS [NOT] DISTINCT FROM NULL` and
`IS [NOT] NULL` with a refcursor-typed expression os the left operand.
There are a few places where CRDB internally expects this comparison to
be valid as well, so disallowing these comparisons has caused test
failures.

This patch removes the refcursor-specific check for invalid comparisons
during type checking, and relies on execution-time checks as is done for
other comparisons. This means that `IsNull` and `IsNotNull` expressions
can now be executed. In addition, the `IS DISTINCT FROM NULL` comparison
is now supported only for the case of type `REFCURSOR` as the left operand,
and `UNKNOWN` (NULL) as the right operand.

Fixes cockroachdb#112010
Fixes cockroachdb#112011

Release note: None
@DrewKimball DrewKimball force-pushed the refcursor-comparison branch from 753606d to 1c2d813 Compare October 9, 2023 21:56
Copy link
Collaborator Author

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)


pkg/sql/logictest/testdata/logic_test/refcursor line 514 at r2 (raw file):

typed NULLs,

Oh, good find. Done.

@DrewKimball
Copy link
Collaborator Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 10, 2023

Build succeeded:

@craig craig bot merged commit 559008f into cockroachdb:master Oct 10, 2023
3 checks passed
@DrewKimball DrewKimball deleted the refcursor-comparison branch October 10, 2023 00:46
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