[6.0.2] Query: Defer inline-ing owned navigation expansion till all joins are generated #26617
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
reviewed
Nov 11, 2021
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
maumar
reviewed
Nov 11, 2021
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
smitpatel
force-pushed
the
smit/patchysnow2
branch
2 times, most recently
from
3c9222b
to
b80e291
Compare
roji
reviewed
Nov 11, 2021
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
| @@ -1208,179 +1212,459 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
|
|
|||
| private Expression? TryExpand(Expression? source, MemberIdentity member) | |||
| { | |||
| source = source.UnwrapTypeConversion(out var convertedType); | |||
| if (source is not EntityShaperExpression entityShaperExpression) | |||
| if (_useOldBehavior) | |||
There was a problem hiding this comment.
Quirk nit: maybe consider just quirking the two (?) sites where things actually changed, rather than duplicating the entire code (would make the diff seem easier to review and much less menacing 😉). If not, maybe consider renaming the method TryExpandOld and leaving as-is, and have a new TryExpand which calls into it if _useOldBehavior.
There was a problem hiding this comment.
I tried the first way but there were multiple sites and certain variables which occur only in one path. I split with if so that it is also easier to see what is the final code and easier to merge to release.
https://github.com/dotnet/efcore/pull/26617/files?w=1 should remove old path indentation diff.
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
|
Risk is M rather than low so fix in main and wait to see more reports. |
|
Is this really medium risk? This issue is going to probably require us to downgrade our application from .NET 6 to .NET 5. =/ |
… generated Issue: Expanding owned navigations in relational falls into 3 categories - Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression - Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level - Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point. In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values. This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated. We also find updated entity projection through binding after we generate a join if pushdown was required. Resolves #26592 The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass. The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
smitpatel
force-pushed
the
smit/patchysnow2
branch
from
b80e291
to
065699a
Compare
smitpatel
changed the title
AndriySvyryd
approved these changes
Dec 8, 2021
AndriySvyryd
changed the title
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.
Issue: Expanding owned navigations in relational falls into 3 categories
In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.
This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.
Resolves #26592
The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.
The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
Description
In a query if owned reference navigations mapped to separate tables are expanded at different time with a subquery causing operation in between (like skip/take/distinct), then the columns of already expanded reference navigation are incorrect.
Customer impact
Query with such scenario will generate invalid SQL causing an exception.
How found
Customer reported on 6.0 package.
Regression
Partially. If the query doesn't have any form of client eval then it fails in 5.0 with same error. If it has client eval then it translated in 5.0 with inefficient SQL.
Testing
Added test for the scenario. Also there are existing tests to validate expansion working correctly in usual cases.
Risk
Medium. This PR changes how owned navigations are expanded in relational layer. While we have good test coverage, this is still significant impact code path. We have added quirk in order to revert to previous behavior if any issue arises.