Query: Convert unflattened GroupJoin to correlated subquery #28998
Conversation
smitpatel
force-pushed
the
smit/groupjoin
branch
from
f2caabf to
fc405f8
Compare
src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| { | ||
| protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression) | ||
| { | ||
| if (methodCallExpression.Method.DeclaringType == typeof(Queryable) |
There was a problem hiding this comment.
Consider returning early it this isn't GroupJoin, to unindent the rest of the method body.
| outerKeySelector.Parameters[0], | ||
| resultSelector.Parameters[0], | ||
| Expression.AndAlso( | ||
| Infrastructure.ExpressionExtensions.CreateEqualsExpression( |
There was a problem hiding this comment.
This seems to check if the outer key is null - do we support that scenario?
There was a problem hiding this comment.
Join doesn't match when keys are null even if outer/inner both evaluate to null.
| { | ||
| var result = Visit(expression); | ||
|
|
||
| return _groupJoinConvertingExpressionVisitor.Visit(result); |
There was a problem hiding this comment.
Can we avoid double visitation here by calling into the GroupJoin conversion logic from VisitMethod (of QueryableMethodNormalizingExpressionVisitor)?
There was a problem hiding this comment.
We cannot. Flattening of GJ-SM requires visited children and visiting it before flattening will remove GJ.
There was a problem hiding this comment.
Ah I see, was not aware that this same visitor also does GJ flattening. We could do something fancy by havnig a pending GroupJoin bubble up and then either flatten it on SelectMany, or do the new conversion logic if not. But that may be too much.
| @@ -132,14 +132,13 @@ public virtual void Throws_when_join() | |||
| } | |||
|
|
|||
| [ConditionalFact] | |||
| public virtual void Throws_when_group_join() | |||
| public virtual void Does_not_throws_when_group_join() | |||
There was a problem hiding this comment.
Can probably remove this with all the other positive tests no?
smitpatel
force-pushed
the
smit/groupjoin
branch
from
8c8269b to
23ee340
Compare
| { | ||
| if (methodCallExpression.Method.DeclaringType == typeof(Queryable) | ||
| && methodCallExpression.Method.IsGenericMethod | ||
| && methodCallExpression.Method.GetGenericMethodDefinition()== QueryableMethods.GroupJoin) |
There was a problem hiding this comment.
| && methodCallExpression.Method.GetGenericMethodDefinition()== QueryableMethods.GroupJoin) | |
| && methodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.GroupJoin) |
smitpatel
force-pushed
the
smit/groupjoin
branch
from
23ee340 to
7fb4578
Compare
Resolves #19930