Query: lift GroupBy aggregates over reference navigations into pre-GroupBy joins

[ducmerida]

Fixes #27933
Fixes #27077

Summary

GroupBy aggregates whose selectors traverse reference navigations currently translate to a correlated scalar subquery that re-scans the source once per group. This PR expands those navigations onto the pre-GroupBy source inside navigation expansion, restoring the EF Core 6 single LEFT JOIN … GROUP BY shape:

ctx.Orders.GroupBy(o => o.EmployeeID)
          .Select(g => new { max = g.Max(i => i.Customer.Region) })

-- before (7.0 → main)                          -- after (= EF Core 6)
SELECT (                                        SELECT MAX([c].[Region]) AS [max]
    SELECT MAX([c].[Region])                    FROM [Orders] AS [o]
    FROM [Orders] AS [o0]                       LEFT JOIN [Customers] AS [c]
    LEFT JOIN [Customers] AS [c] ON …               ON [o].[CustomerID] = [c].[CustomerID]
    WHERE [o].[EmployeeID] = [o0].[EmployeeID]  GROUP BY [o].[EmployeeID]
        OR (… IS NULL AND … IS NULL)) AS [max]
FROM [Orders] AS [o]
GROUP BY [o].[EmployeeID]

Real-world motivation: a production P&L report (fact table grouped subsidiary × month, 15 sums, two traversing navigations) went from ~1,500 per-group rescans to one join; all 9 aggregate grand totals are identical digit-for-digit (decimal columns) against the app's previous NHibernate implementation.

Background

The #27931 PR description states the regression and the plan verbatim: "Side-effect above is that joins expanded over the grouping element (due to navigations used on aggregate chain), doesn't translate to aggregate anymore since we need to translate the join on parent query, remove the translated join if the chain didn't end in aggregate and also de-dupe same joins. Filing issue to improve this in future." — that issue is #27933.

It also warns "Due to fragile nature of matching to lift the join, we shouldn't try to lift joins" — that verdict is about matching translated SQL trees in the relational layer (the EF6 lifting removed for the #27132 bugs). This PR never touches translated trees: the join comes from re-running navigation expansion itself over the pre-GroupBy source, so the pushdown/alias-uniquification fragility that killed relational lifting structurally cannot occur.

One additional finding (verified empirically): the GroupBy(key, elementSelector) overload does not avoid the subquery either — the element selector is recorded as a pending selector and the navigation re-surfaces inside the per-group clone. There is no LINQ-side workaround short of writing the join manually; the join this PR produces is the one navigation expansion would have produced for that manual form.

Implementation

The lift lives in NavigationExpandingExpressionVisitor (per review feedback; the earlier preprocessing visitor is deleted), gated by a provider capability flag:

• INavigationExpansionExtensibilityHelper.SupportsNavigationExpansionJoins — default interface implementation returning false; the new RelationalNavigationExpansionExtensibilityHelper implements it as true. Non-relational providers (Cosmos, InMemory) keep today's translation untouched.
• ProcessGroupBy stashes the pre-GroupBy parent (NavigationExpansionExpression) and the original key selector on GroupByNavigationExpansionExpression. Operators applied over the grouping sequence clear the stash (UpdateSource), so HAVING-like shapes keep today's translation, as before.
• ProcessSelect(GroupByNavigationExpansionExpression, …) scans the result selector for aggregates whose selector traverses a reference navigation; when found, it re-expands the key and aggregate selectors on the stashed parent via the existing ExpandNavigationsForSource, then rebuilds GroupBy(key).Select(aggregates) over the joined source.
• Because expansion is the standard machinery, everything it already knows comes for free: join type from the model (LEFT for optional, INNER for required navigations — an improvement over the always-LEFT synthesized GroupJoin), join dedup across aggregates and across key and aggregate (which is what fixes GroupBy on Navigation Property not translating properly #27077), composite-key navigations, global query filters landing on the join exactly as they land inside today's subquery (IgnoreQueryFilters() clears both identically), and provider query-root semantics — temporal AS OF sources join temporal principals via SqlServerNavigationExpansionExtensibilityHelper.CreateQueryRoot.
• Correctness argument: the navigation join is row-preserving by construction (FK → unique principal key ⇒ at most one match per row), so sibling aggregates, Count() and the grouping key are unaffected; the per-group rowset is identical to the one inside today's correlated subquery (which also joins the navigation), so all aggregate values including null semantics are unchanged.
• Shapes that don't qualify fall through to today's translation unchanged: owned-type paths (no join to add), non-aggregate uses of the group besides g.Key, operators between GroupBy and Select, the GroupBy(key, elementSelector) overload, and non-whitelisted aggregates.

Testing

• Golden baseline GroupBy_with_aggregate_through_navigation_property updated to the EF6 shape. Baselines changed across the suites, all to the lifted single-join shape, all results-verified: GearsOfWar base/TPT/TPC/Temporal on SQL Server (the temporal variant now lifts with FOR SYSTEM_TIME AS OF propagated to every joined table), GearsOfWar on Sqlite, Ef6.GroupBy query-syntax samples, NorthwindGroupBy (including a required navigation now producing INNER JOIN), two UdfDbFunction TVF tests, AdHocMiscellaneous repeated aggregates, and ComplexNavigationsSharedType composite-key shapes.
• New specification tests (all providers; lifted baselines on SQL Server): multiple aggregates sharing one navigation (deduped single join), two-level navigation chain, Count with a navigation predicate, aggregate over a navigation hidden in an intermediate projection, navigation shared by key and aggregate sharing one join (GroupBy on Navigation Property not translating properly #27077), and filtered navigations — filter on the join, with an additional unfiltered Count() unaffected by it, and with IgnoreQueryFilters().
• Full local runs: EFCore.SqlServer.FunctionalTests 50,801 tests and EFCore.Sqlite.FunctionalTests 38,358 tests — remaining failures are environment-only (Full-Text Search / memory-optimized / SqlAzure not installed locally; a preview-runtime BCL artifact), zero related to this change. InMemory unchanged (flag off).

Notes for reviewers

• The lift only fires when an aggregate selector actually traverses a reference navigation, so flat-aggregate translations are byte-for-byte unchanged.
• Explicit non-goals, tracked separately: EF Core 7 GroupBy with Owned Entity perf Regression #29593 (owned types — the correlation there originates in the relational GroupBy fast path, not in navigation expansion, so the lift is a no-op for it), key-side expansion (Expand navigations on grouping key #29014, GroupBy entity subquery #29013, GroupBy owned entity #29018, GroupBy over complex key generates redundant subquery #30113), group materialization (GroupBy with a projection of the IGrouping ToArray or other collection results in a inefficient sql query #35991). Improves Redundant inner join when I'm using GroupBy() #28622 (redundant join) and GroupBy possibly causes needless duplication of predicate #29670 (predicate duplication gone wherever the fallback is no longer taken). Possible follow-up: lifting the GroupBy(key, elementSelector) overload.

[ducmerida]

@dotnet-policy-service agree

[Copilot]

Pull request overview

This PR improves relational query translation performance by rewriting GroupBy(...).Select(g => aggregate(g, x => x.ReferenceNav.Prop)) patterns into pre-GroupBy left joins, avoiding the per-group correlated subquery re-scan regression introduced after EF Core 6 (Fixes #27933).

Changes:

• Introduces a relational-only LINQ rewriter (GroupByAggregateNavigationLiftingExpressionVisitor) to lift reference-navigation access inside aggregate selectors into explicit pre-GroupBy joins.
• Hooks the rewriter into RelationalQueryTranslationPreprocessor.Process() so it runs before GroupJoin-flattening/normalization.
• Updates/adds SQL baselines and new spec tests covering multiple aggregates sharing a navigation, two-level navigation chains, predicate Count, and intermediate projections.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/EFCore.Relational/Query/RelationalQueryTranslationPreprocessor.cs	Runs the new lifting visitor early in relational preprocessing.
src/EFCore.Relational/Query/Internal/GroupByAggregateNavigationLiftingExpressionVisitor.cs	Implements the LINQ-level rewrite to synthesize pre-GroupBy left joins for reference-navigation aggregate selectors.
test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs	Adds new cross-provider spec tests for the lifted-join scenarios.
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs	Updates SQL baselines and adds SQL Server assertions for the new tests.
test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs	Updates TVF-related GroupBy aggregate SQL baselines to the lifted-join shape.
test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs	Updates GearsOfWar SQL baselines affected by the new translation shape.
test/EFCore.SqlServer.FunctionalTests/Query/Inheritance/TPTGearsOfWarQuerySqlServerTest.cs	Updates TPT SQL baseline for the lifted-join translation.
test/EFCore.SqlServer.FunctionalTests/Query/Inheritance/TPCGearsOfWarQuerySqlServerTest.cs	Updates TPC SQL baseline for the lifted-join translation.
test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs	Updates Sqlite SQL baseline for the lifted-join translation.

[AndriySvyryd]

If you'd rather see this inside navigation expansion proper (expand-on-parent with deferred GroupBy materialization), this visitor still works as an executable specification — shapes, bail-outs and target SQL are all pinned by the tests, which transfer wholesale.

Yes, the correlated subquery is born in nav expansion, not in a relational translator.

The "expand on the parent" (smitpatel's original plan in #27933) literally means: expand the aggregate's navigation on the pre-GroupBy source, bake the join into the source as a transparent identifier, group over the widened element, and rewrite the aggregate to read the joined TI member. That's exactly what the EF6 shape was.

The decisive advantage over the PR's relational pre-pass is reuse of existing, correct machinery instead of re-deriving it, reducing the number of changes siginficantly.

The one real downside: this is Core, not Relational

Moving the lift into core is only safe if it's gated so providers don't see synthesized joins if they aren't supported.

The existing extensibility point INavigationExpansionExtensibilityHelper can be used to gate the whole feature behind a new capability flag SupportsNavigationExpansionJoins there, default false, enabled by relational providers.

Proposed implementation plan:

Step 0 — Gating flag on the extensibility helper

• Add INavigationExpansionExtensibilityHelper.SupportsNavigationExpansionJoins => false.
• Add a relational base helper that overrides it to true and register it in EntityFrameworkRelationalServicesBuilder; Cosmos already has its own registration path and keeps the core default false.

Step 1 — Carry the pre-GroupBy source forward

Modify GroupByNavigationExpansionExpression:

• Today it stores the finished GroupBy call in Source plus the per-group GroupingEnumerable. Add a reference to the parent NavigationExpansionExpression (the source that was grouped) and the expanded key selector lambda, so ProcessSelect can go back before the group boundary to add joins and then rebuild the GroupBy.

Modify ProcessGroupBy (resultSelector == null branch):

• When the flag is off, unchanged.
• When on, still build the same GroupByNavigationExpansionExpression, but also stash the parent NavigationExpansionExpression (pre-group source, its CurrentTree, PendingSelector, CurrentParameter and the expanded keySelector on it. Do not eagerly widen here — we don't yet know which navigations the aggregates need.

Step 2 — The core rewrite in ProcessSelect(GroupByNavigationExpansionExpression, …)

This is where the correlation is currently created. New flagged path, executed before the existing clone-and-expand logic:

1. Scan the result-selector body for whitelisted aggregates over the grouping parameter: Sum/Min/Max/Average(selector) and Count/LongCount(predicate). Reuse the recognition shape from the PR's GroupingUsageScanner but as a local helper. Bail (fall through to today's path) if the grouping is used for anything other than g.Key / a whitelisted aggregate.
2. Collect distinct reference-navigation paths used inside those selectors/predicates (o => o.Customer.City, od => od.Order.Customer.City). Only single-valued navigations qualify; a path ending in a navigation, or any collection/owned nav, makes that aggregate ineligible → fall through.
3. For each distinct navigation prefix, expand it on the parent using the existing pipeline:
   • Build a member-access lambda parent => parent.<navPrefix> against the parent's PendingSelector, and run ExpandNavigationsForSource(parentSource, …). This produces the FK→PK left join and returns a NavigationExpansionExpression whose CurrentTree now contains a transparent identifier for the joined principal.
   • Crucially, Reduce it into the source (as ProcessSelectMany/ProcessJoin do) so the join is materialized in Source and the joined entity is addressable via a stable TI member — not left as a pending selector. (The pending-deferral is exactly why the GroupByWithKeyElementSelector overload doesn't already fix this, per the PR's own finding.)
   • Dedupe: paths sharing a prefix reuse the same widened source (join dedup falls out naturally because expansion of the same navigation on the same tree is idempotent / caches).
4. Rebuild the GroupBy over the widened element type: remap the stored key selector onto the new TI parameter (walk .Outer hops, same helper Expand() already uses at the top of the file), emit GroupByWithKeySelector on the widened source.
5. Rewrite each aggregate so its selector/predicate reads the joined TI member instead of navigating (t => t.<TI>.Inner.City), then emit the aggregate over the new IGrouping. Sibling aggregates that don't traverse navigations, g.Count(), and g.Key are unaffected because the added join is row-preserving (FK → unique PK).
6. Return the resulting NavigationExpansionExpression (a plain GroupBy(...).Select(...) over the widened source), which flows through the rest of the pipeline normally.

Guardrails inside this path (fall through to today's translation on any):

• grouping used outside g.Key/aggregate;
• non-single-valued or path-ends-in-navigation;
• g.Select(...).Distinct().Agg() (SUM(DISTINCT) — later);
• operators between GroupBy and Select (HAVING stays correlated).

Note: composite/shadow FK, owned, temporal, inheritance, and query-filter handling are not guardrails here — they're handled correctly by ExpandNavigationsForSource/ApplyQueryFilter, which is the whole point.

Step 3 — Count/LongCount with a nav predicate

Modify ProcessAllAnyCountLongCount(GroupByNavigationExpansionExpression, …): when the flag is on and the predicate traverses a reference navigation, route it through the same widen-parent logic (share a helper with Step 2) so g.Count(o => o.Customer.City == "London") becomes a widened COUNT(CASE …) over the joined column instead of a correlated predicate.

Step 4 — Intermediate one-level projection

The PR's "Shape 2" (src.Select(o => new { o.EmployeeID, o.Customer.City }).GroupBy(k).Select(aggs)) already works more naturally here: ProcessSelect(NavigationExpansionExpression,…) records the projection as the PendingSelector, so by the time ProcessGroupBy runs, the parent's PendingSelector already exposes City. The nav path is visible to Step 2's scan without a separate inlining pass — validate with the PR's GroupBy_aggregate_through_navigation_in_intermediate_projection test.

Step 5 — Finalization (Expand) and GroupingElementReplacingExpressionVisitor

• The GroupByNavigationExpansionExpression tail block in Expand that materializes the pending selector via GroupByWithKeyElementSelector must be consistent with the widened element shape. Since Step 2 returns a fully-formed NavigationExpansionExpression (not a GroupByNavigationExpansionExpression) when the lift fires, that block is simply not reached for lifted queries — confirm the lifted path returns before the deferred-GroupBy machinery.
• GroupingElementReplacingExpressionVisitor needs no change for lifted aggregates (they never go through the clone path); leave it entirely for the fall-through (unlifted) cases.
• GroupByAggregateNavigationLiftingExpressionVisitor can now be deleted.

Step 6 — Tests

• The four new specification tests from the PR transfer wholesale (they're provider-agnostic): multiple aggregates sharing one nav, two-level chain, Count with nav predicate, nav in intermediate projection.
• Add the coverage the PR lacks, now that it's meaningful:
  • a principal with a global query filter + an IgnoreQueryFilters() variant (proves filter placement on the synthesized join);
  • an inheritance hierarchy principal (proves the FindEntityType class of bug can't recur);
  • a composite/owned case asserting it still translates (via existing expansion) or cleanly falls through.
• Baselines: SqlServer/Sqlite converge on the same single LEFT JOIN … GROUP BY shape the PR produces; Cosmos and InMemory baselines stay unchanged (flag off / no-join).

[ducmerida]

Thanks a lot @AndriySvyryd — plan implemented and pushed. The preprocessing visitor is now deleted (which also moots both earlier Copilot comments), the lift lives in ProcessSelect(GroupByNavigationExpansionExpression, …) plus a stash on GroupByNavigationExpansionExpression, and the only relational-layer change left is the capability helper.

Findings from the implementation:

Your reuse argument held up empirically, beyond what I expected:

1. Temporal now lifts correctly — the exact case that forced a bail-out in the pre-pass (a plain synthesized root joined to an AS-OF source returned wrong rows) now translates to a single grouped query with FOR SYSTEM_TIME AS OF propagated to every joined table (TemporalGearsOfWarQuerySqlServerTest, results verified). SqlServerNavigationExpansionExtensibilityHelper.CreateQueryRoot does it for free, as you said.
2. Composite-key navigations lift (ComplexNavigationsSharedTypeQuerySqlServerTest.GroupBy_aggregate_where_required_relationship and friends) — the pre-pass bailed on those outright.
3. Required navigations produce INNER JOIN (the synthesized GroupJoin/DefaultIfEmpty was always-LEFT); and shapes the syntactic pre-pass never matched now lift: query-syntax (Ef6…Whats_new_2021_sample_7/9), GroupBy-in-subquery (Complex_query_with_groupBy_in_subquery4), repeated correlated aggregates (AdHoc…GroupBy_Aggregate_over_navigations_repeated). All results-verified; baselines updated.
4. GroupBy on Navigation Property not translating properly #27077 is now fixed as a by-product: a navigation used in both the key and an aggregate now shares one join — pinned by a new test (GroupBy_key_and_aggregate_through_same_navigation), since key and aggregate expansions hit the same parent tree and expansion is idempotent.
5. Query filters: added the coverage you asked for — the principal's filter lands on the join exactly as inside today's subquery, and IgnoreQueryFilters() clears both (NorthwindQueryFiltersQueryTestBase, with and without).

Steps 3–4 folded away: Count/LongCount with a navigation predicate is recognized by the same scanner (no ProcessAllAnyCountLongCount change — grouping-sequence operators clear the stash via UpdateSource and keep today's translation), and the intermediate-projection shape needs no inlining pass — the parent's pending selector already exposes the navigation path by ProcessSelect time.

Owned paths cleanly fall through to today's translation, as the plan specified — the existing correlated baselines (OwnedQuerySqlServerTest.GroupBy_aggregate_on_owned_navigation_in_aggregate_selector and friends) pass unchanged.

Validation

Full EFCore.SqlServer.FunctionalTests (50,801) and EFCore.Sqlite.FunctionalTests (38,358) runs clean (environment-only failures); InMemory unchanged (flag off). The lift only fires when an aggregate selector actually traverses a reference navigation, so flat-aggregate translations are byte-for-byte unchanged.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

[AndriySvyryd]

Thanks for your contribution!

[github-actions]

API review baseline changes for src/EFCore.Relational/EFCore.Relational.baseline.json

Show diff

The diff below was generated by ApiChief between the base and the PR.

+ class Microsoft.EntityFrameworkCore.Query.RelationalNavigationExpansionExtensibilityHelper :
+     Microsoft.EntityFrameworkCore.Query.NavigationExpansionExtensibilityHelper, Microsoft.EntityFrameworkCore.Query.INavigationExpansionExtensibilityHelper
+ RelationalNavigationExpansionExtensibilityHelper(NavigationExpansionExtensibilityHelperDependencies dependencies);

API review baseline changes for src/EFCore/EFCore.baseline.json

Show diff

The diff below was generated by ApiChief between the base and the PR.

  interface Microsoft.EntityFrameworkCore.Query.INavigationExpansionExtensibilityHelper
+ bool SupportsNavigationExpansionJoins { get; }