-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Query: Navigation Expansion take 2 #16785
Conversation
smitpatel
commented
Jul 27, 2019
- Add TransparentIdentifier to uniquify them across the pipeline
- Convert all Enumerable based method which we can translate to Queryable
- Add QueryableMethodProvider to get methodInfos on Queryable robustly
- Split out subquery member pushdown out of navigation expansion
- Lastly expand navigations
This is still WIP. I will complete the work next week. Just wanted to get it out to get early feedback. How it works (on high level)
While this has not fixed any issue, it lays groundwork for many
|
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a partial review - I haven't gone over the core of the changes yet but will hopefully do that very soon.
src/EFCore/Query/Internal/SubqueryMemberPushdownExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/Internal/SubqueryMemberPushdownExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to re-target to release/3.0 before merging.
This is ready for review. |
@AndriySvyryd - Also removed calling subquery member pushdown twice. Now it runs only after collection navigation is expanded and generating key access over FirstOrDefault. |
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.Expressions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Relational.Specification.Tests/TestUtilities/TestSqlLoggerFactory.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Show resolved
Hide resolved
Everything passed. If anyone wants to give a re-review, feel free. I will wait till tomorrow noon for merge. |
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.Expressions.cs
Show resolved
Hide resolved
- Add TransparentIdentifier to uniquify them across the pipeline - Convert all Enumerable based method which we can translate to Queryable - Add QueryableMethodProvider to get methodInfos on Queryable robustly - Split out subquery member pushdown out of navigation expansion - Lastly expand navigations