Query: support composing over GroupBy + First per group (navigations and joins)#38687
Open
ducmerida wants to merge 4 commits into
Open
Query: support composing over GroupBy + First per group (navigations and joins)#38687ducmerida wants to merge 4 commits into
ducmerida wants to merge 4 commits into
Conversation
…vigation expansion ProcessSelect over a grouping built the resulting navigation tree from Expression.Default, so the entity produced by a cardinality-reduced group element (GroupBy(k).Select(g => g.OrderBy(...).First())) lost its entity reference and any navigation accessed on it afterwards failed with the misleading "Translation of member ... failed. This commonly occurs when the specified member is unmapped." Snapshot the selector structure before reducing, the same way ProcessDistinct preserves its pending selector, so navigations on the result can still be expanded.
… composed over Composing a join or an entity re-projection over GroupBy(k).Select(g => g.First()) failed with "ProjectionBindingExpression could not be translated": the projection binder had no handling for a client projection slot holding a single-result subquery, and its lowering only happened at the end of translation in ApplyProjection. Extract the single-result to-one lowering from ApplyProjection into a shared core (parameterized on the projection sink) and invoke it on demand from the projection binder, pushing the select down first and remapping the binder's already-collected projections with the pushdown's remapping visitor. Fixes dotnet#28125
Navigation OrderBy/Where/projection on the group element entity and a manual join over it, following the GroupBy_Select_Entire_Entity family; skipped on InMemory like the rest of the family (dotnet#31209).
There was a problem hiding this comment.
Pull request overview
This PR fixes translation failures when composing further query operators over the entity produced by a “top-1-per-group” pattern (GroupBy(...).Select(g => g.OrderBy(...).First()/FirstOrDefault())), especially when later traversing navigations or performing joins. It does so by preserving entity shape through navigation expansion and by enabling on-demand lowering of single-result subqueries during relational projection binding.
Changes:
- Navigation expansion now snapshots selector structure before reducing cardinality, preserving entity references for later navigation expansion.
- Relational
SelectExpressionextracts/reuses the single-result subquery lowering logic and exposes an internal on-demand lowering hook used by the projection binder when a single-result subquery is composed over. - Adds new spec + SQL Server functional tests for navigation
OrderBy/Where/projection and a manualJoinover the per-groupFirst()element; skips these in InMemory consistently with the existing family.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs | Adds SQL assertions for the new GroupBy+First-per-group composition shapes on SQL Server. |
| test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs | Adds new cross-provider specification tests covering navigation and join composition over the grouped “first” entity. |
| test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs | Skips the new tests for InMemory in line with existing GroupBy/entire-entity limitations. |
| src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs | Preserves selector structure across cardinality reduction to keep entity references for later navigation expansion. |
| src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs | Refactors single-result subquery lowering into a shared core and adds an internal on-demand lowering entry point for projection binding. |
| src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs | Invokes on-demand lowering when binding a single-result subquery projection so composed queries can continue translating. |
The lowering mutates the select expression (pushdown + to-one join), so a projection referencing the same slot twice would have added a duplicate join of the same inner select. Cache the lowered shaper per slot in the projection binder and add a test pinning the single-join shape.
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
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.
Fixes #38686
Fixes #28125
Summary
Composing over the entity produced by a top-1-per-group query failed in two independent layers:
Expression.Default, so the group element entity lost its entity reference and no navigation on it could be expanded (Query: Re-evaluate concept of pending selector in nav expansion #20291 territory).ApplyProjection— "we don't allow this terms to compose (yet!)".Implementation
NavigationExpandingExpressionVisitor.ProcessSelect(GroupBy…)now snapshots the selector structure before reducing (same mechanismProcessDistinctuses for its pending selector), so a cardinality-reduced group element keeps its entity reference and later navigation accesses expand into regular joins over the grouped query.ApplyProjectioninto a shared core (LowerSingleResultSubqueryCore, parameterized on the projection sink — behavior-identical for the existing deferred path) and the projection binder now invokes it on demand when a composed query binds a single-result slot: pushdown if needed (remapping the binder's already-collected projections with the pushdown's remapping visitor), prunable to-oneOUTER APPLY, shaper remap. Since the lowering mutates the select expression, the binder memoizes the lowered shaper per projection slot (same idiom as its existing_projectionBindingCache), so a projection referencing the slot more than once reuses the single join.Scalar member access after the per-group
First()already worked (#26748 fixed the second-projection case;OrderByby scalar translates as a correlated scalar subquery). This PR adds the missing entity-level composition. Known follow-up: the join key over the group element still binds as a correlated scalar subquery in theONclause instead of reusing the lowered join's column — correct but suboptimal (the duplication described in #20291).All currently-failing shapes throw today, so no existing SQL baselines change.
Aware of #37859/#32957 (moving navigation expansion into translation, 12.0): the navigation-expansion piece is a targeted fix for the 11 pipeline; the on-demand lowering lives on the translation side, aligned with that direction, and the new tests pin the behavior for the rewrite.
Testing
GroupBy_Select_Entire_Entityfamily: navigationOrderBy/Where/projection over the group element, a manualJoinover it (The LINQ expression 'ProjectionBindingExpression: 0/1/2' could not be translated. #28125), and a projection referencing the group element twice (whose baseline pins the single-join shape). Results-verified baselines on SQL Server; skipped on InMemory like the rest of the family (When using GroupBy, getting "the given key 'EmptyProjectionMember' was not present in the dictionary" #31209).EFCore.SqlServer.FunctionalTests(50,818),EFCore.Sqlite.FunctionalTests(38,375),EFCore.InMemory.FunctionalTests(28,230) — remaining failures are environment-only (Full-Text Search, memory-optimized, SqlAzure, PrimitiveCollections BCL-preview artifact); InMemory fully clean.