Fix to #28648 - Json/Query: translate element access of a json array#29656
Merged
Fix to #28648 - Json/Query: translate element access of a json array#29656
Conversation
maumar
commented
Nov 23, 2022
maumar
commented
Nov 23, 2022
maumar
force-pushed
the
fix28648_do_deduplication_separately
branch
8 times, most recently
from
517e7c7 to
f0daace
Compare
roji
reviewed
Nov 24, 2022
src/EFCore.Relational/Query/Internal/CollectionIndexerToElementAtConvertingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs
Outdated
Show resolved
Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Show resolved
Hide resolved
maumar
force-pushed
the
fix28648_do_deduplication_separately
branch
7 times, most recently
from
a67d570 to
f852a75
Compare
smitpatel
reviewed
Nov 30, 2022
...EFCore.Relational/Query/Internal/CollectionIndexerToElementAtNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
maumar
force-pushed
the
fix28648_do_deduplication_separately
branch
from
f852a75 to
3f1b691
Compare
Contributor
Author
|
new version up @roji |
roji
reviewed
Dec 4, 2022
Member
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
LGTM, lots of comments but almost all nits. Would prefer we didn't exempt byte[] from the ElementAt normalization though...
...EFCore.Relational/Query/Internal/CollectionIndexerToElementAtNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...EFCore.Relational/Query/Internal/CollectionIndexerToElementAtNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| var subquery = mcne.Subquery; | ||
|
|
||
| if (subquery is MethodCallExpression selectMethodCall | ||
| && selectMethodCall.Method.IsGenericMethod |
Member
There was a problem hiding this comment.
Can integrate into pattern above (also block below)
| // MCNE(JSON_QUERY($[0].Collection)) | ||
| // MCNE(JSON_QUERY($[0].Collection).AsQueryable()) | ||
| // MCNE(JSON_QUERY($[0].Collection).Select(xx => xx.Includes()) | ||
| // MCNE(JSON_QUERY($[0].Collection).AsQueryable().Select(xx => xx.Includes()) |
Member
There was a problem hiding this comment.
Can we also have AsQueryable after the Select? If so that's not handled below (can just do a while loop to unwrap until nothing is left).
Contributor
Author
There was a problem hiding this comment.
we don't produce those kinds of expressions - we add asqueryable when the collection is List or other Enumerable. So its always either AsQueryable().Select() or just Select(). Of course customers could also write queries like that (but why?)
Member
There was a problem hiding this comment.
OK, thanks for clarifying, no problem.
maumar
force-pushed
the
fix28648_do_deduplication_separately
branch
2 times, most recently
from
3999868 to
8f76e65
Compare
Contributor
Author
|
new version up @roji |
Converting indexer over list/array into ElementAt, so that nav expansion understands it and can perform pushown and inject MaterializeCollectionNavigation expression where necessary. In translation phase (specifically in ExpandSharedTypeEntities) we recognize the pattern that nav expansion creates and if the root is JsonQueryExpression, we apply collection index over it. JsonQueryExpression path segment now consists of two components - string representing JSON property name and SqlExpression representing collection index (it can be constant, parameter or any arbitrary expression that resolves to int) Deduplication is heavily restricted currently - we only de-duplicate projections whose additional path consists of JSON property accesses only (no collection indexes allowed). All queries projecting entities that need JSON array access must be set to NoTracking (for now). This is because we don't flow those collection index values into shaper. Instead, the ordinal keys are filled with dummy values, which prohibits us from doing proper change tracking. Fixes #28648
maumar
force-pushed
the
fix28648_do_deduplication_separately
branch
from
8f76e65 to
94be38c
Compare
roji
approved these changes
Dec 5, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Converting indexer over list/array into ElementAt, so that nav expansion understands it and can perform pushown and inject MaterializeCollectionNavigation expression where necessary. In translation phase we recognize the pattern that nav expansion creates and if the root is JsonQueryExpression, we apply collection index over it. JsonQueryExpression path segment now consists of two components - string representing JSON property name and SqlExpression representing collection index (it can be constant, parameter or any arbitrary expression that resolves to int)
Deduplication is heavily restricted currently - we only de-duplicate projections whose additional path consists of JSON property accesses only (no collection indexes allowed). All queries projecting entities that need JSON array access must be set to NoTracking (for now). This is because we don't flow those collection index values into shaper. Instead, the ordinal keys are filled with dummy values, which prohibits us from doing proper change tracking.
Fixes #28648