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

Parentheses put around IS NULL only if operand is of type boolean (6.0) #26652

Closed
roji opened this issue Nov 12, 2021 · 9 comments · Fixed by #26653
Closed

Parentheses put around IS NULL only if operand is of type boolean (6.0) #26652

roji opened this issue Nov 12, 2021 · 9 comments · Fixed by #26653
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Nov 12, 2021

#25722 fixed #23990 by adding parentheses around IS NULL/IS NOT NULL, preventing operator precedence issues. However, it did so only when the operand of IS NULL is a bool. However, the same problem can happen if a non-bool column is being checked for null.

See npgsql/efcore.pg#2090 on PostgreSQL where the following LINQ query:

var boolParam = true;
_ = ctx.Blogs.Where(c => boolParam != c.NullableGuid.HasValue).ToList();

... generates the following SQL ...

SELECT b."Id", b."NullableGuid"
FROM "Blogs" AS b
WHERE @__boolParam_0 <> b."NullableGuid" IS NOT NULL

... which fails with operator does not exist: boolean <> uuid, because in PostgreSQL the precedence of <> is higher than the precedence of IS NOT NULL (docs). The issue also causes wrong data to be returned in Sqlite.

@ajcvickers
Copy link
Contributor

@roji You put servicing-consider on this. Are you planning to get it into 6.0.1?

@ajcvickers ajcvickers assigned roji and smitpatel and unassigned roji Nov 16, 2021
@ajcvickers ajcvickers added this to the 7.0.0 milestone Nov 16, 2021
@roji roji removed this from the 7.0.0 milestone Nov 16, 2021
@roji
Copy link
Member Author

roji commented Nov 16, 2021

@ajcvickers @smitpatel sorry, I confused this issue with something else in triage; the reason I put servicing-consider on this is that it makes SQLite return incorrect data (because of bad operator precedence). A patch specifically for this (without full-on general parentheses support) would be quite easy and risk-free.

@roji
Copy link
Member Author

roji commented Nov 16, 2021

For the SQLite issue:

SELECT TRUE <> (NULL IS NULL); -- returns 0
SELECT TRUE <> NULL IS NULL; -- return 1

@smitpatel
Copy link
Contributor

@roji - Can you file a separate issue for 7.0 fix? (You can assign to me if you want)

roji added a commit to roji/efcore that referenced this issue Nov 19, 2021
@roji
Copy link
Member Author

roji commented Nov 19, 2021

@smitpatel is that basically #23895?

@smitpatel
Copy link
Contributor

That issue has been fixed. You would need to file a new issue for this particular query if we plan to fix it differently.

roji added a commit to roji/efcore that referenced this issue Nov 19, 2021
@roji roji changed the title Parentheses put around IS NULL only if operand is of type boolean Parentheses put around IS NULL only if operand is of type boolean (6.0) Nov 19, 2021
@roji
Copy link
Member Author

roji commented Nov 19, 2021

Well we'll definitely need to... I'd track it all under #23895 (correct parentheses around IS NULL, remove unneeded parentheses around AND/OR), but if you want to file something different go ahead.

@smitpatel
Copy link
Contributor

#23895 is about adding mechanism which is already done.

@roji
Copy link
Member Author

roji commented Nov 19, 2021

Filed #26767. You probably need another one for the AND/OR simplification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants