Lift GroupBy aggregates through filtered principals as outer joins

[benedict-odonovan]

Fixes #38965

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format
• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo
  Follow-up to [release/11.0] Preserve GroupBy groups through filtered required navigations #38973, which fixed GroupBy aggregate over a required navigation with a query filter drops entire groups (Regression in 11.0) #38965 on release/11.0 by declining to lift.

Description

#38668 lifts a GroupBy aggregate's navigation traversal into a join beneath the GROUP BY. When the navigation is required that join is an INNER JOIN, and a required FK guarantees a matching principal row but not that the row survives the principal's query filter — so the join removed dependent rows, and with them whole groups, including the value of sibling aggregates that never touched the navigation.

#38973 restores correctness by declining to lift for that shape, which also gives up the optimization for it. This restores the optimization instead: where a join would remove rows because the principal is filtered, it is emitted as an outer join, so no source row is removed and no group is lost.

Before (correlated subquery per group):

SELECT [d].[GroupId] AS [Key], COUNT(*) AS [Count], (
    SELECT MAX([p0].[Value])
    FROM [Dependents] AS [d0]
    INNER JOIN (
        SELECT [p].[Id], [p].[Value]
        FROM [Principals] AS [p]
        WHERE [p].[Filtered] = CAST(0 AS bit)
    ) AS [p0] ON [d0].[PrincipalId] = [p0].[Id]
    WHERE [d].[GroupId] = [d0].[GroupId]) AS [MaxValue]
FROM [Dependents] AS [d]
GROUP BY [d].[GroupId]

After:

SELECT [d].[GroupId] AS [Key], COUNT(*) AS [Count], MAX([p0].[Value]) AS [MaxValue]
FROM [Dependents] AS [d]
LEFT JOIN (
    SELECT [p].[Id], [p].[Value]
    FROM [Principals] AS [p]
    WHERE [p].[Filtered] = CAST(0 AS bit)
) AS [p0] ON [d].[PrincipalId] = [p0].[Id]
GROUP BY [d].[GroupId]

The decision is made per foreign key while expanding an aggregate selector, in ExpandForeignKey itself, rather than from a scan of the selector: an unfiltered principal in the same projection keeps its inner join, and a filtered one is relaxed however the selector happens to reach it. That matters for shapes a scan does not recognise — EF.Property(d.Principal, "Value") is not a member chain, and today loses its group exactly as in #38965 whenever some other aggregate is what enables the lift.

The row an outer join keeps

An outer join does not reproduce the correlated subquery on its own. Where the subquery had no row at all for a filtered-out principal, the join keeps one null-extended row, and an expression which turns that null into a value can see it — g.Sum(d => ((int?)d.Principal.Value) ?? 5) would contribute 5 to a group that must contribute nothing, and All(), which counts the rows whose predicate is not true, would answer false where the subquery answers true.

So each lifted aggregate is built over grouping.Where(e => principal != null), which excludes that row from that aggregate. Count() with no selector is not guarded and still counts every row in the group — the point of the fix.

The guard covers exactly the joins this handling relaxed, recorded by ExpandForeignKey as it makes them. Two consequences worth stating, because both are easy to get wrong:

• A chain relaxes more than one principal — d.Middle.Leaf.Value with filters on both — and the guard covers every one of them. Relaxing the middle is what makes the leaf's join outer, so those nulls belong to this handling too.
• A required filtered principal behind an ordinary optional navigation is not guarded. Its join is outer whatever this does, its nulls are visible to every other translation of the same query, and guarding it here would answer differently from them. GroupBy_aggregate_over_filtered_principal_behind_an_optional_navigation pins that.

The guard is skipped where the aggregate cannot observe the row anyway: when the selector is nothing but a column read off a relaxed principal, modulo casts (e => (int?)e.Principal.Value), the null lands in MAX/MIN/SUM/AVG/COUNT(predicate)/Any(predicate) and folds away. All() is always guarded, since a null predicate is not true and would flip its answer. Across the tests here, ten lifted aggregates in six tests carry a guard; every other one emits the plain aggregate above.

A general "is this selector null-propagating?" analysis would remove a few more guards, but the property has to hold of the SQL while the analysis runs on the LINQ tree, before translation — CONCAT absorbs nulls on SQL Server where + propagates them, mapped DbFunctions do whatever they were written to do — and a wrong answer is silent wrong results. The narrow rule above is the part that is sound without knowing the provider.

A note on the detector

With the relaxation decided at the join, ReferenceNavigationAccessDetector no longer needs to know about query filters: its FoundFilteredRequiredNavigation and requiredPath walk had no remaining callers, so it goes back to answering only "does this selector traverse a reference navigation", which is what decides whether lifting is worthwhile. That removes the filter-awareness added in #38973 — not because anything was wrong with it, but because the decision it fed now happens where the join is made, and leaving two places to keep in step is how the misses above happened.

Equivalence

Every aggregate kind that lifts was compared against the correlated-subquery translation on the same data. For a group whose only row has a filtered-out principal both produce Count = 1, Sum = 0 (ISNULL(SUM(...), 0)), Average = null, Min/Max = null, Count(predicate) = 0, Any(predicate) = false, All(predicate) = true. A non-nullable Average over such a group throws Cannot read the Value property of a Nullable object in both translations, unchanged.

Scoping

The join type is decided in ExpandForeignKey, which every navigation expansion shares, so the relaxation is scoped to the ExpandingExpressionVisitor that expands the aggregate selectors and is deliberately not carried into the nested expansions it triggers. Without that confinement, a principal whose own query filter reaches through a required navigation of its own and is null-tolerant (p => p.Category.DeletedOn == null) would have that filter's join relaxed too, and a principal in a deleted category would pass its own filter when reached from an aggregate. GroupBy_aggregate_over_required_navigation_keeps_the_principals_own_filter_exact covers it.

Testing

Added to the #region 38965 in AdHocQueryFiltersQueryRelationalTestBase, with SQL Server and SQLite baselines:

Test	Covers
..._aggregates_of_every_kind_...	Sum, Average, Min, Count(predicate), Any, Count in one projection
..._with_null_observing_selector	?? 5 and ?? 0 in a selector and a predicate: the kept row must not be visible
..._All_over_required_navigation_...	All() stays vacuously true over a group whose principals were all filtered out
..._reaching_the_principal_through_EF_Property	a selector that is not a member chain
..._lifted_by_an_unfiltered_sibling	the same, where nothing in the projection reaches a filtered principal by a member chain
..._over_chain_with_two_filtered_principals	both hops filtered and both relaxed: the guard covers both
..._behind_an_optional_navigation	model-optional hop: not relaxed, not guarded, answer unchanged
..._keeps_the_principals_own_filter_exact	nested filter expansions keep their inner join
..._filtered_and_unfiltered_required_navigations	LEFT JOIN and INNER JOIN side by side
..._over_optional_navigation_...	optional navigation, already outer-joined
..._key_over_filtered_required_navigation_removes_the_rows	grouping by a filtered principal's column
..._over_chain_with_query_filter_on_the_far_principal	two-hop chain, filter on the far end only
..._over_self_referencing_navigation_...	principal and dependent are the same entity type
..._to_TPH_/_TPT_/_TPC_principal_...	filter on the hierarchy root under each mapping strategy
..._aggregates_sharing_one_filtered_navigation	two aggregates, one join
..._over_filtered_navigation_after_Take	the join stays on top of the composed source

The inheritance, chain, self-reference, sharing and Take shapes were added to probe other translations #38668 might have changed; all of them behaved correctly before and after this change, so they are coverage rather than fixes. Temporal is already pinned by #38668's own baseline (INNER JOIN [Cities] FOR SYSTEM_TIME AS OF ...), and this change alters join type rather than query-root creation.

Full SQL Server query suite green, and no existing baseline outside this region moved.

[Copilot]

🟡 Changes recommended

Outer-join lifting can introduce synthetic null-extended rows that change aggregate and predicate results; the critical issue remains unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR restores GroupBy aggregate lifting through filtered required principals using scoped outer joins, with expanded relational test coverage and SQL baselines.

Changes:

• Adds filter-aware outer-join expansion.
• Preserves correlated translation for All.
• Adds coverage for filters, chains, inheritance, self-references, and Take.
• Updates SQL Server and SQLite baselines.

File summaries

File	Summary
test/EFCore.SqlServer.FunctionalTests/Query/AdHocQueryFiltersQuerySqlServerTest.cs	SQL Server baselines
test/EFCore.Sqlite.FunctionalTests/Query/AdHocQueryFiltersQuerySqliteTest.cs	SQLite baselines
test/EFCore.Relational.Specification.Tests/Query/AdHocQueryFiltersQueryRelationalTestBase.cs	Behavioral tests and models
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs	Filter-aware join selection
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs	Aggregate lifting and filter detection

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Two moderate correctness findings remain unresolved: multi-hop filtered principals are incompletely guarded, and EF.Property navigation shapes are not guarded.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs:1929

• The access detector only records a filtered principal while walking a member chain rooted at a NavigationTreeExpression. EF.Property<int?>(d.Principal, "Value") ?? 5 (and EF.Property<Principal>(d, "Principal").Value) reaches the same navigation expansion but does not produce such a chain, so if another aggregate enables lifting this selector is outer-joined without a guard and contributes 5 instead of an empty aggregate. Include these supported EF.Property navigation shapes in the recorded accesses, or decline lifting when they cannot be classified.

                    access = Expression.MakeMemberAccess(access, member);

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Two moderate issues remain in filtered-principal join tracking and EF.Property navigation handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:504

• relaxed is true even when entityReference.IsOptional is already true because an earlier model relationship was optional. That makes a required filtered principal behind an ordinary optional navigation look like a fix-created non-row-preserving join whenever a sibling aggregate enables leftJoinFilteredPrincipals; the resulting guard removes a null-extended row that the correlated subquery intentionally exposes to selectors such as ?? 5. Restrict this relaxation/recording to joins whose optionality was introduced by the filtered-principal handling, not merely to required FKs with filtered targets.

            var relaxed = leftJoinFilteredPrincipals
                && !derivedTypeConversion
                && IsRelaxableFilteredPrincipal(foreignKey, onDependent);

            var innerJoin = requiredJoin && !relaxed;

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs:1790

• Using traversesFilteredPrincipal here does not cover every selector that ExpandNavigationsForSource can expand. ReferenceNavigationAccessDetector only recognizes member chains; EF.Property<int?>(d.Principal, "Value") visits just the one-member d.Principal and leaves this flag false. If a sibling aggregate over an unfiltered navigation is what enables the lift, this selector is still lifted with an INNER JOIN to the filtered required principal, so its group disappears. Enable relaxation based on the actual expanded joins (or make detection cover EF.Property/indexer paths), while continuing to derive the guard from the joins recorded by ExpandForeignKey.

                leftJoinFilteredPrincipals: traversesFilteredPrincipal,

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The query-translation changes have complex navigation, aggregate, and provider-specific semantics requiring final human review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite