Fix to #19499 - Query: null semantics not applied on subquery.Contains(null) #19744
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
force-pushed
the
fix19499
branch
2 times, most recently
from
af8fa81
to
509e08a
Compare
|
ping |
smitpatel
reviewed
Feb 21, 2020
src/EFCore.Relational/Query/NullabilityBasedSqlProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
smitpatel
reviewed
Feb 21, 2020
| && methodCallExpression.Method.GetGenericMethodDefinition() is MethodInfo containsMethodInfo | ||
| && containsMethodInfo.Equals(QueryableMethods.Contains) | ||
| && methodCallExpression.Arguments[0].NodeType is ExpressionType containsFirstArgumentNodeType | ||
| && (containsFirstArgumentNodeType != ExpressionType.Constant || ((ConstantExpression)methodCallExpression.Arguments[0]).IsEntityQueryable()) |
There was a problem hiding this comment.
is ConstantExpression would remove the explicit cast.
smitpatel
reviewed
Feb 21, 2020
| && containsMethodInfo.Equals(QueryableMethods.Contains) | ||
| && methodCallExpression.Arguments[0].NodeType is ExpressionType containsFirstArgumentNodeType | ||
| && (containsFirstArgumentNodeType != ExpressionType.Constant || ((ConstantExpression)methodCallExpression.Arguments[0]).IsEntityQueryable()) | ||
| && containsFirstArgumentNodeType != ExpressionType.Parameter |
There was a problem hiding this comment.
use Expression directly rather than ET type enum. It is mixed here.
smitpatel
reviewed
Feb 21, 2020
|
rebase. |
maumar
force-pushed
the
fix19499
branch
2 times, most recently
from
48bb523
to
3d23171
Compare
|
@smitpatel new version up |
|
@smitpatel ping |
smitpatel
reviewed
Mar 3, 2020
src/EFCore.Relational/Query/NullabilityBasedSqlProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
smitpatel
reviewed
Mar 3, 2020
src/EFCore.Relational/Query/NullabilityBasedSqlProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
smitpatel
approved these changes
Mar 3, 2020
…s(null) We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null. Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics. Also it allows us to translate contains on entities with composite keys. Also made several small fixes: - marked EXISTS as never nullable for purpose of null semantics, - marked IN as never nullable for purpose of null semantics when both the subquery projection element and the item expression are not nullable, - optimized EXISITS (subquery) and IN (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits and doesn't contain any results, - improves expression printer output for IN expression in the subquery scenario.
smitpatel
reviewed
Mar 4, 2020
| if (methodCallExpression.Method.IsGenericMethod | ||
| && methodCallExpression.Method.GetGenericMethodDefinition() is MethodInfo containsMethodInfo | ||
| && containsMethodInfo.Equals(QueryableMethods.Contains) | ||
| && !(methodCallExpression.Arguments[0] is ParameterExpression) |
There was a problem hiding this comment.
What are these conditions on parameter/constant doing here? I removed them an no test failed.
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.
We used to translate this into
NULL IN subquerypattern, but this doesn't work because it doesn't match if the subquery also contains null.Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.
Also made several small fixes: