Query: Introduce client projections in SelectExpression #24675
Conversation
test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
| @@ -391,525 +393,622 @@ public void ApplyDistinct() | |||
| /// <summary> | |||
| /// Adds expressions from projection mapping to projection if not done already. | |||
| /// </summary> | |||
| public void ApplyProjection() | |||
| public Expression ApplyProjection(Expression shaperExpression, QuerySplittingBehavior querySplittingBehavior) | |||
There was a problem hiding this comment.
No way to preserve old method with new behavior.
| /// <summary> | ||
| /// Clears all existing projections. | ||
| /// </summary> | ||
| public void ClearProjection() |
There was a problem hiding this comment.
Behavior has diverged so there is no good new behavior. Obsoleted to point to how to remove client projections.
(We can add method in future to add scalar projections if we get feedback for it)
| /// Replaces current projection mapping with a new one to change what is being projected out from this <see cref="SelectExpression" />. | ||
| /// </summary> | ||
| /// <param name="projectionMapping"> A new projection mapping. </param> | ||
| public void ReplaceProjectionMapping(IDictionary<ProjectionMember, Expression> projectionMapping) |
| /// </summary> | ||
| /// <param name="projectionMember"> A projection member to search in the mapping. </param> | ||
| /// <returns> The mapped projection for given projection member. </returns> | ||
| public Expression GetMappedProjection(ProjectionMember projectionMember) |
| /// </summary> | ||
| /// <param name="entityProjection"> An entity projection to add. </param> | ||
| /// <returns> A dictionary of <see cref="IProperty" /> to int indicating properties and their corresponding indexes in the projection list. </returns> | ||
| public IReadOnlyDictionary<IProperty, int> AddToProjection(EntityProjectionExpression entityProjection) |
There was a problem hiding this comment.
No way to preserve old behavior
| /// </summary> | ||
| /// <param name="sqlExpression"> An expression to add. </param> | ||
| /// <returns> An int value indicating the index at which the expression was added in the projection list. </returns> | ||
| public int AddToProjection(SqlExpression sqlExpression) |
There was a problem hiding this comment.
This still exists below.
| /// </summary> | ||
| /// <param name="shapedQueryExpression"> A shaped query expression for the subquery producing single non-scalar result. </param> | ||
| /// <returns> A shaper expression to shape the result of this projection. </returns> | ||
| public Expression AddSingleProjection(ShapedQueryExpression shapedQueryExpression) |
There was a problem hiding this comment.
No way to preserve old behavior
| /// <param name="navigation"> A navigation associated with this collection, if any. </param> | ||
| /// <param name="elementType"> The type of the element in the collection. </param> | ||
| /// <returns> A <see cref="CollectionShaperExpression" /> which represents shaping of this collection. </returns> | ||
| public CollectionShaperExpression AddCollectionProjection( |
There was a problem hiding this comment.
No way to preserve old behavior
| /// <param name="elementType"> The type of the element in the collection. </param> | ||
| /// <param name="splitQuery"> A value indicating whether the collection query would be run with a different DbCommand. </param> | ||
| /// <returns> An expression which represents shaping of this collection. </returns> | ||
| public Expression? ApplyCollectionJoin( |
There was a problem hiding this comment.
No way to preserve old behavior
|
/cc @Emill, according to @smitpatel this should help with your work on array-based includes. Might be better to wait until this is merged before tackling the EF side... |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I ran through the new code using the debugger, and from what I see it seems very nice indeed as a base for the collection-as-array approach! I guess what simply has to be done now is to skip most of the code in ApplyProjection that transforms the collection into a join, and replace it with an array of rows projection instead. The materializer code should be quite similar to what I've already done, but hopefully more clean when reading out the projection items. Not sure though how the |
Let's have this discussion on the EFCore.PG repo, to avoid create PG-specific noise here (unless you think it could impact the design in this PR in some way). |
|
Sure, just brought up the topic here since it relates to the change in this PR how to reference a specific projection with the new "client projection" index thing. |
|
@Emill - The rough idea is that an enumerable subquery which pg specifically want to convert to a different SQL, now can be intercepted in ShapedQueryExpression before projection is applied as whole. So you can get that associated projection out and modify it as needed to form the custom expression and replace |
|
|
- ClientProjections is alternate when ProjectionMember binding is not possible using indexes - ClientProjections is separate from SelectExpression.Projection where former holds complex client side mapped projections and later holds only scalars - What it enables - Single result subquery is not joined right away - pendingCollections are removed - Ability to bind with above subquery translations after projection has converted to index based binding (enabling pending selector future change) - Ability to bind client projections smoothly like ProjectionMember binding so suppose translations post-client eval where it is supported - Grouping.FirstOrDefault can add subquery projection which applies separately so we can combine aggregate/non-aggregate projection on grouping - Introduce CollectionResultExpression which holds info on how to create collection for a subquery - ApplyProjection converts projectionMember/Index based bindings to actual scalar projection and updates shaper in same pass - Unique collectionId are assigned by shaper - Change in flow to projection to let sqlTranslator determines translatibility before processing for client side - Regression in collection over single result subquery - Earlier we applied single result right away copying over pending collection to outer which allowed single result subquery to convert apply to join - Now all client projections apply at same time so pending collection (equivalent subquery) is applied inside single result subquery so we fail to convert apply to join Ground work for #20291, #12088, #13805 Resolves #23571
|
Hello @smitpatel! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit … nested This fixes slight regression which was introduced in #24675 We need to lift projections for single result since if there is a split query on it then we need it joined to top level to get proper parent query to clone
… nested This fixes slight regression which was introduced in #24675 We need to lift projections for single result since if there is a split query on it then we need it joined to top level to get proper parent query to clone
Ground work for #20291, #12088, #13805
Resolves #23571