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

[ducmerida]

Fixes #27933

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 rewrites them into explicit pre-GroupBy left joins, 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: it synthesizes the join at the LINQ level, before translation, from model metadata, 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; that manual explicit-join form is exactly what this PR synthesizes.

Implementation

GroupByAggregateNavigationLiftingExpressionVisitor (new, EFCore.Relational, internal) rewrites
src.GroupBy(k).Select(g => …aggregates…) into
src.GroupJoin(...).SelectMany(p => p.Inners.DefaultIfEmpty(), ...).GroupBy(k′).Select(…)
when aggregate selectors traverse reference navigations:

• One left join per distinct navigation prefix (multi-level chains become chained hops; paths sharing a prefix share the hop — join dedup across aggregates). Pair types come from TransparentIdentifierFactory.
• Also handles src.Select(proj).GroupBy(k).Select(aggs) by inlining the one-level projection first.
• Hooked in RelationalQueryTranslationPreprocessor.Process — relational-only (Cosmos must never see synthesized joins) and before QueryableMethodNormalizingExpressionVisitor, whose existing GroupJoin-flattening turns the synthesized pattern into a LeftJoin. No changes to navigation expansion or the relational translators.
• 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 left-joins), so all aggregate values including null semantics are unchanged. Global query filters on the joined principal land on the synthesized join exactly as they land inside today's subquery, and IgnoreQueryFilters() affects both identically.
• Conservative bail-outs keep today's translation for: composite/shadow FKs, owned types, collection navigations, paths ending in a navigation, non-whitelisted aggregates, any non-aggregate group usage besides g.Key (including g.Select(...).Distinct().Agg() — SUM(DISTINCT) support is a possible follow-up), operators between GroupBy and Select (HAVING stays correlated, as today), and sources with non-plain query roots or provider-specific queryable operators — the full-suite run caught TemporalAsOf sources joining a plain (non-AS-OF) synthesized root and returning wrong rows; the SourceSafetyScanner bail-out now guards that entire class of mismatch.

Testing

• Golden baseline GroupBy_with_aggregate_through_navigation_property updated to the EF6 shape; across the suites seven baselines changed, all to the lifted single-join shape, all with results verified (GearsOfWar base/TPT/TPC on SQL Server, GearsOfWar on Sqlite, and two UdfDbFunction TVF tests — the temporal GearsOfWar variant intentionally keeps its original translation via the bail-out).
• Four new specification tests (all providers; lifted baselines on SQL Server): multiple aggregates sharing one navigation (baseline shows the deduped single join), two-level navigation chain, Count with a navigation predicate, aggregate over a navigation hidden in an intermediate projection.
• Full local runs: EFCore.SqlServer.FunctionalTests 50,751 tests and EFCore.Sqlite.FunctionalTests 38,330 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.

Notes for reviewers

• 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.
• Explicit non-goals, tracked separately: EF Core 7 GroupBy with Owned Entity perf Regression #29593 (owned types), 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 GroupBy on Navigation Property not translating properly #27077 (aggregate-side subquery gone; a duplicate join against key-side expansion may remain — inferred, not reproduced) and GroupBy possibly causes needless duplication of predicate #29670 (predicate duplication gone wherever the fallback is no longer taken).

[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.