Navigation expansion rewrite - first draft #15383
Conversation
| @@ -11,6 +11,7 @@ Microsoft.EntityFrameworkCore.DbSet | |||
| <MinClientVersion>3.6</MinClientVersion> | |||
| <AssemblyName>Microsoft.EntityFrameworkCore</AssemblyName> | |||
| <RootNamespace>Microsoft.EntityFrameworkCore</RootNamespace> | |||
| <NoWarn>$(NoWarn);CS1591</NoWarn> | |||
There was a problem hiding this comment.
disabled for now, too much work to put those in, and the codebase is in flux
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class CustomRootExpression : Expression, IPrintable |
There was a problem hiding this comment.
this represents a root that is not a EntityQueryable, e.g. anonymous projection after Distinct.
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class IncludeExpression : Expression, IPrintable |
There was a problem hiding this comment.
Should we just produce IncludeMethod expression call instead? That's what this gets reduced to anyway
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public static class LinqMethodHelpers |
There was a problem hiding this comment.
this might be useful in other places, not necessarily just for nav rewrite - move it to more common place?
There was a problem hiding this comment.
We don't need actual method infos for queryable methods. We just need to know which method it is. A loose matching can easily tell that without much computation.
For Enumerable methods, we may need them for InMemory server side. But it should not be required in pre-compilation phase. If it is necessary then again loose match is pretty safe option.
In reply to: 276042273 [](ancestors = 276042273)
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class NavigationBindingExpression : Expression, IPrintable |
There was a problem hiding this comment.
this represents navigation property chains (or just "naked" parameter expression). As we expand the navigations, adding joins we keep updating source expression and the mapping. This way we don't need to re-create entire expression trees, but only mutate the mappings themselves. Once the given subexpression is processed, the binding gets rewritten to appropriate property access expressions which are immutable.
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class NavigationExpansionExpression : Expression, IPrintable |
There was a problem hiding this comment.
represents IQueryable/IEnumerable that has it's navigations expanded. Operand is the actual expression produces so far (with the extra joins/groupjoins that were added), NavigationExpansionExpressionState holds all metadata we gathered along the way.
src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs
Outdated
Show resolved
Hide resolved
| MaterializeCollectionNavigation = materializeCollectionNavigation; | ||
| } | ||
|
|
||
| public virtual ParameterExpression CurrentParameter { get; set; } |
There was a problem hiding this comment.
current parameter (either original or transparent identifier) for the IQueryable/IEnumerable. This should always be the parameter for the pending selector
| } | ||
|
|
||
| public virtual ParameterExpression CurrentParameter { get; set; } | ||
| public virtual List<SourceMapping> SourceMappings { get; set; } |
There was a problem hiding this comment.
all the EntityQueryable sources found so far - there can be multiple sources due to join/groupjoin
|
|
||
| public virtual ParameterExpression CurrentParameter { get; set; } | ||
| public virtual List<SourceMapping> SourceMappings { get; set; } | ||
| public virtual LambdaExpression PendingSelector { get; set; } |
There was a problem hiding this comment.
pending selector is what we need to apply to the NavigationExpansionExpression at the end so that the final shape matches the expected shape - it collapses all TransparentIdentifiers into actual entities etc
| public virtual List<SourceMapping> SourceMappings { get; set; } | ||
| public virtual LambdaExpression PendingSelector { get; set; } | ||
| public virtual bool ApplyPendingSelector { get; set; } | ||
| public virtual List<(MethodInfo method, LambdaExpression keySelector)> PendingOrderings { get; set; } |
There was a problem hiding this comment.
we need to apply orderings lazily because cases like: customers.OrderBy(c => c.Nav.Name).ThenBy(c => c.Id), if we apply first order by right away, we will introduce Join for the Nav expansion and lose the IOrderedQueryable type, which will break ThenBy. We need to apply Joins but hold off with the orderings until we encounter node that is NOT a ThenBy
| public virtual LambdaExpression PendingSelector { get; set; } | ||
| public virtual bool ApplyPendingSelector { get; set; } | ||
| public virtual List<(MethodInfo method, LambdaExpression keySelector)> PendingOrderings { get; set; } | ||
| public virtual NavigationBindingExpression PendingIncludeChain { get; set; } |
There was a problem hiding this comment.
this helps when processing ThenInclude - we want to know what we included before so we can build the full include chain easier
| public virtual bool ApplyPendingSelector { get; set; } | ||
| public virtual List<(MethodInfo method, LambdaExpression keySelector)> PendingOrderings { get; set; } | ||
| public virtual NavigationBindingExpression PendingIncludeChain { get; set; } | ||
| public virtual MethodInfo PendingCardinalityReducingOperator { get; set; } |
There was a problem hiding this comment.
FirstOrDefault, etc - this is applied lazily as well to help with member pushdown
| public virtual NavigationBindingExpression PendingIncludeChain { get; set; } | ||
| public virtual MethodInfo PendingCardinalityReducingOperator { get; set; } | ||
| public virtual List<string> PendingTags { get; set; } | ||
| public virtual List<List<string>> CustomRootMappings { get; set; } |
There was a problem hiding this comment.
mappings for elements that are not EntityQueryables and therefore not part of SourceMapping structure - those are custom roots (e.g. anonymous projections after Distinct) and nested queryables, e.g. GroupJoin groupings or nested collections projections
| public virtual MethodInfo PendingCardinalityReducingOperator { get; set; } | ||
| public virtual List<string> PendingTags { get; set; } | ||
| public virtual List<List<string>> CustomRootMappings { get; set; } | ||
| public virtual INavigation MaterializeCollectionNavigation { get; set; } |
There was a problem hiding this comment.
do we need to materialize a navigation of a given type - yes if "naked" collection navigation is projected, false otherwise
| materializeCollectionNavigation?.ClrType ?? operand.Type); | ||
| } | ||
|
|
||
| public static (Expression source, ParameterExpression parameter) AddNavigationJoin( |
There was a problem hiding this comment.
adds joins for all the reference navigations that were found but not expanded yet. It also changes all the mappings to new values
There was a problem hiding this comment.
TODO: most of this code is copied from old nav rewrite, can probably be optimized
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class NavigationExpansionRootExpression : Expression, IPrintable |
There was a problem hiding this comment.
represents a nested navigation expansion that needs it's own mapping to the root - e.g. for groupjoin scenarios, where the grouping is the Outer element of the Transparent identifier created in the GJ result selector
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion | ||
| { | ||
| public class NavigationTreeNode |
There was a problem hiding this comment.
represents the tree of navigations that were discovered so far as well as the mappings to and from. To mappings are way to locate the given navigation in the current transparent identifier (Outer/Inner props), From mappings are ways that the given navigation can be accessed originally. There can be multiple from mappings for a given navigation, because queries like these: customers.Select(c => new { one = c, two = c })
| return result; | ||
| } | ||
|
|
||
| public List<NavigationTreeNode> Flatten() |
There was a problem hiding this comment.
helper to give us all the children - this is mostly used to quickly check if some processing is needed in the tree, e.g. if there are any nodes with pending expansions
| using System.Linq.Expressions; | ||
| using Microsoft.EntityFrameworkCore.Extensions.Internal; | ||
|
|
||
| public class PendingSelectorIncludeRewriter : ExpressionVisitor |
There was a problem hiding this comment.
adds include methods to the final selector with the correct nesting of the include calls. e.g.
customers.Include(c => Foo).Include(c => Bar) will be rewritten to Include(Include(c, Foo), Bar).
customers.Include(c => Foo).ThenInclude(f => Bar) will be rewritten to Include(c, Include(f, Bar)),
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| public class NavigationComparisonOptimizingVisitor : NavigationExpansionVisitorBase |
There was a problem hiding this comment.
rewrites nav comparison to key comparison instead
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| public partial class NavigationExpandingVisitor : ExpressionVisitor |
There was a problem hiding this comment.
main navigation expansion processor - visits all the (currently supported) linq methods and converts them into NavigationExpansionExpressions, which can later be reduced into result expressions with their navigations expanded
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| public partial class NavigationExpandingVisitor : ExpressionVisitor |
There was a problem hiding this comment.
TODO: will combine this with the file above - just split them temporarily
| case "ThenInclude": | ||
| return ProcessInclude(methodCallExpression); | ||
|
|
||
| //TODO: should we have relational version of this? - probably |
There was a problem hiding this comment.
We need relational version. We need to make it extensible so that providers can add custom queryable method processing
In reply to: 276049367 [](ancestors = 276049367)
There was a problem hiding this comment.
will do that once we have the proper way to inject the visitors in the new pipeline
| methodCallExpression.Type); | ||
| } | ||
|
|
||
| throw new InvalidOperationException("collection selector was not NavigationExpansionExpression"); |
There was a problem hiding this comment.
TODO: need to do something better here. we should always have NEE as a result of visiting SelectMany collection selector, unless query has some method that is not supported (maybe client method, or if we missed something)
| // TODO: need to create the new state and copy includes from the old one, rather than simply copying it over to grouping | ||
| // this shouldn't be a problem currently since we don't support queries that compose on the grouping | ||
| // but when we do, state can't be shared - otherwise any nav expansion that affects the flattened part of the GroupJoin would be incorrectly propagated to the grouping as well | ||
| var newGrouping = new NavigationExpansionExpression(newGroupingParameter, innerApplyOrderingsResult.state, groupingParameter.Type); |
There was a problem hiding this comment.
fixed in the second commit
| return new NavigationExpansionExpression(rewritten, preProcessResult.state, methodCallExpression.Type); | ||
| } | ||
|
|
||
| private (Expression source, NavigationExpansionExpressionState state) PreProcessTerminatingOperation(NavigationExpansionExpression source) |
There was a problem hiding this comment.
for terminating operators, like Distinct we need to "flush" all the accumulated stare - bring the result to the shape that is expected (i.e. get rid of TransparentIdentifiers) and then recreate new sources from the entities projected before the operator.
e.g. customers.Where(c => c.Nav.Name != "Foo").Select(c => new { foo = c, bar = c }.Distinct().Where(x => ...)
at the time of processing Where we will have two separate query sources for customer entity. We will also lose the navigation expanded in the first where, so if the second where accesses it again, it needs to be expanded the second time.
| return new NavigationExpansionExpression(applyOrderingsResult.source, applyOrderingsResult.state, methodCallExpression.Type); | ||
| } | ||
|
|
||
| private MethodCallExpression SimplifyPredicateMethod(MethodCallExpression methodCallExpression, bool queryable) |
There was a problem hiding this comment.
converts methods like entities.Count(x => x.Name != "Foo") into entities.Where(x => x.Name != "Foo").Count()
| return (source, state); | ||
| } | ||
|
|
||
| private (Expression source, Expression lambdaBody, NavigationExpansionExpressionState state) FindAndApplyNavigations( |
There was a problem hiding this comment.
takes a (single parameter) lambda from the queryable method that we are processing, finds all the new navigations and applies the joins for them
| var pendingSelector = state.PendingSelector; | ||
| if (state.CurrentParameter != result.parameter) | ||
| { | ||
| var |
There was a problem hiding this comment.
TODO - add tests for those, correct implementations is in the Include visitors, since logic is similar
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| public class NavigationExpansionVisitorBase : ExpressionVisitor |
There was a problem hiding this comment.
TODO: convert this into visit children on individual expressions
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| class NavigationPropertyBindingVisitor : NavigationExpansionVisitorBase |
There was a problem hiding this comment.
parses the property access chains and tries to match it with a NavigationTreeNode, based on nodes' FromMappings. If can't find a match it creates a new node at the correct place in the tree.
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
| class NavigationPropertyUnbindingVisitor : NavigationExpansionVisitorBase |
There was a problem hiding this comment.
converts navigation bindings back into actual member access chains, also reduces navigation expansion expressions (in case of nested scenarios) - this is usually a final operation we do when processing lambda of a some query operator
| namespace Microsoft.EntityFrameworkCore.Query.NavigationExpansion.Visitors | ||
| { | ||
|
|
||
| public class PendingIncludeFindingVisitor : ExpressionVisitor |
There was a problem hiding this comment.
finds all the pending includes that are actually accessible from the final projection - users could have included some navigations that are not accessible (e.g. behind scalar projection or not present at all) - we don't want to include those
| @@ -5533,6 +5556,8 @@ public virtual Task SelectMany_navigation_property_followed_by_select_collection | |||
| [MemberData(nameof(IsAsyncData))] | |||
| public virtual Task Multiple_SelectMany_navigation_property_followed_by_select_collection_navigation(bool isAsync) | |||
| { | |||
| isAsync = false; | |||
| } | ||
|
|
||
|
|
||
| //[ConditionalFact] |
| @@ -1960,7 +1984,7 @@ where e1.FirstName | |||
| entryCount: 1); | |||
| } | |||
|
|
|||
| [ConditionalTheory(Skip = "Issue #14935. Cannot eval 'where ([e1].FirstName == {from Employee e in value(Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1[Microsoft.EntityFrameworkCore.TestModels.Northwind.Employee]) orderby [e].EmployeeID asc select new <>f__AnonymousType334`1(Foo = [e]) => FirstOrDefault()}.Foo.FirstName)'")] | |||
| [ConditionalTheory] | |||
| @@ -62,7 +62,8 @@ public override async Task Key_equality_using_property_method_required2(bool isA | |||
| AssertSql( | |||
| @"SELECT [l].[Id], [l].[Date], [l].[Level1_Optional_Id], [l].[Level1_Required_Id], [l].[Name], [l].[OneToMany_Optional_Inverse2Id], [l].[OneToMany_Optional_Self_Inverse2Id], [l].[OneToMany_Required_Inverse2Id], [l].[OneToMany_Required_Self_Inverse2Id], [l].[OneToOne_Optional_PK_Inverse2Id], [l].[OneToOne_Optional_Self2Id] | |||
| FROM [LevelTwo] AS [l] | |||
| WHERE [l].[Level1_Required_Id] > 7"); | |||
| INNER JOIN [LevelOne] AS [l.OneToOne_Required_FK_Inverse2] ON [l].[Level1_Required_Id] = [l.OneToOne_Required_FK_Inverse2].[Id] | |||
There was a problem hiding this comment.
we no longer do entity.Navigation.Pk -> entity.FK optimization, so there will be additional joins in some places
maumar
force-pushed
the
nav_exp_pr_take1
branch
2 times, most recently
from
935eb65 to
7000602
Compare
| @@ -107,6 +114,7 @@ public virtual TResult Execute<TResult>(Expression query) | |||
| var queryContext = _queryContextFactory.Create(); | |||
|
|
|||
| query = ExtractParameters(query, queryContext, _logger); | |||
| query = ExpandNavigations(query); | |||
There was a problem hiding this comment.
This needs to be inside CompileQueryCore methods. You are expanding navigations for cached query too. That would be disastrous perf.
maumar
force-pushed
the
nav_exp_pr_take1
branch
from
7000602 to
708f48c
Compare
|
|
maumar
force-pushed
the
nav_exp_pr_take1
branch
from
708f48c to
a18290d
Compare
Known limitations: - GroupBy: not supported at all - Include tracking/fixup: include method (one that suppose to do fixup and tracking) is just a stub currently - only does fixup one way and doesn't perform tracking at all, - Various warnings and negative cases: we used to warn/throw particular exceptions for negative cases, those are now different and the messages themselves are not hardcoded and not polished - include/project collection - correlated collection feature is disabled, so projecting collections will issue N+1 queries. Same goes for include collection. - async + collections - no special handling of async, projecting/including collection may cause a deadlock
maumar
force-pushed
the
nav_exp_pr_take1
branch
from
a18290d to
1dfb651
Compare
|
Note: this currently lives outside the Pipeline directory. |
| return ProcessMemberPushdown(newExpression, navigationExpansionExpression, efProperty: false, memberExpression.Member, propertyName: null, memberExpression.Type); | ||
| } | ||
|
|
||
| return base.VisitMember(memberExpression); |
There was a problem hiding this comment.
Aren't we visiting memberExpression.Expression twice in this method?
Known limitations: