[release/9.0-staging] Fix to #35393 - GroupJoin in EF Core 9 Returns Null for Joined Entities #35448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maumar
commented
Jan 10, 2025
| if (!UseOldBehavior35393) | ||
| { | ||
| // prefer equality in predicates when comparing to constants | ||
| if (optimize && notEqual && left.Type == typeof(bool) && (left is SqlConstantExpression || right is SqlConstantExpression)) |
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/EFCore.Relational/Query/SqlNullabilityProcessor.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs:2484
- [nitpick] The variable name prm is ambiguous. It should be renamed to nullableBoolParam.
var prm = default(bool?);
|
We could also go with a simpler version, which just removes the faulty optimization. There are some other queries which are affected and perf could regress on them. Thoughts? - see #35449 |
Problem was that in EF9 we moved some optimizations from sql nullability processor to SqlExpressionFactory (so that we optimize things early). One of the optimizations: !(true = a) -> false = a !(false = a) -> true = a In principle, this should be valid in 3-value logic a (true = a) !(true = a) (false = a) 0 0 1 1 1 1 0 0 N N N N but not in c# semantics, both null == true and null == false are evaluated to false, so !(true == a) is not the same as false = a (first one is true, second one is false) Fix is to drop this optimization from SqlExpressionFactory and instead compensate in nullability processor (where we have full nullability information), so that we don't regress performance for cases in which this optimization happened to be valid. Fixes #35393
maumar
force-pushed
the
fix35393_90_take2
branch
from
a6a4f9f to
e81aba0
Compare
|
closing in favor of the simpler version |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem was that in EF9 we moved some optimizations from sql nullability processor to SqlExpressionFactory (so that we optimize things early). One of the optimizations:
!(true = a) -> false = a
!(false = a) -> true = a
In principle, this should be valid in 3-value logic
but not in c# semantics, both null == true and null == false are evaluated to false, so !(true == a) is not the same as false = a (first one is true, second one is false)
Fix is to drop this optimization from SqlExpressionFactory and instead compensate in nullability processor (where we have full nullability information), so that we don't regress performance for cases in which this optimization happened to be valid.
Fixes #35393