Whitelist Any() and All() for GroupBy aggregate navigation lifting - #38829
Open
benedict-odonovan wants to merge 1 commit into
Open
Conversation
Author
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR fixes an EF Core 11 regression in GroupBy aggregate navigation lifting where the presence of Any()/All() in a GroupBy result selector caused all sibling aggregates to stop being lifted into the shared pre-GroupBy navigation join (falling back to correlated subqueries). The change expands the allowed “predicate aggregate” whitelist so quantifiers no longer demote unrelated aggregates.
Changes:
- Whitelist
Enumerable.AnyandEnumerable.AllinGroupingAggregateScanner.PredicateAggregateMethodNamesso GroupBy result selectors containing quantifiers don’t mark the grouping parameter as unsupported usage. - Add new specification tests covering
Any/All(Enumerable and Queryable forms) through navigations and alongside other aggregates. - Add/extend SQL Server baselines for the new tests, including query-filtered navigation scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs | Adds Any/All to the predicate-aggregate whitelist used by GroupBy aggregate lifting eligibility checks. |
| test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs | Adds spec tests for Any/All GroupBy quantifiers through navigations and in combination with lifted aggregates. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs | Adds SQL Server expected SQL for the new Northwind GroupBy Any/All scenarios. |
| test/EFCore.Specification.Tests/Query/NorthwindQueryFiltersQueryTestBase.cs | Adds spec tests for GroupBy Any through a filtered navigation, including IgnoreQueryFilters and “with total” variants. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindQueryFiltersQuerySqlServerTest.cs | Adds SQL Server expected SQL baselines for the filtered-navigation Any GroupBy scenarios. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Add Any and All to PredicateAggregateMethodNames in GroupingAggregateScanner so a quantifier in the result selector no longer counts as an unsupported use of the grouping parameter and blocks the lift for every sibling aggregate - Aggregates traversing reference navigations are lifted into the shared pre-GroupBy join again; Any/All keep translating to EXISTS/NOT EXISTS - Add specification tests mirroring the shapes covered by dotnet#38668: predicate through a navigation, Enumerable and Queryable sources, quantifier alongside a lifted aggregate, multiple aggregates sharing one navigation, two-level navigation, navigation shared by key and aggregate, navigation in an intermediate projection, and filtered navigations with and without IgnoreQueryFilters - Add the corresponding SQL Server baselines Fixes dotnet#38816
benedict-odonovan
force-pushed
the
whitelist-any-for-groupby-aggregate-navigation-lifting
branch
from
August 20, 2026 10:23
6c681bb to
bff0805
Compare
Author
|
Rebased the PR on main to solve the nullability warnings/failures on the CI tests 😄 |
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 #38816
Summary
#38668 lifts
GroupByaggregates whose selectors traverse a reference navigation onto a shared pre-GroupByjoin.GroupingAggregateScanneronly allows the grouping parameter to appear asg.Keyor as the source of a whitelisted aggregate;AnyandAllwere not on that whitelist, so any quantifier in the result selector marked the grouping parameter as an unsupported usage and disabled the lift for every aggregate in the group, not just for itself.The trigger is the presence of the quantifier, not anything it reads — a bare
g.Any()or anAllwhose predicate touches no navigation demotes its siblings just the same. The workaround was to writeg.Count(...) > 0instead ofg.Any(...), which shows the shape is otherwise handled.This PR adds
AnyandAlltoPredicateAggregateMethodNames.AnyandAllthemselves still translate to a correlatedEXISTS/NOT EXISTS, which is the right shape for them; what changes is that they no longer demote their siblings.Implementation
One-line change:
AnyandAlljoinCountandLongCountinGroupingAggregateScanner.PredicateAggregateMethodNames.Everything else already works for them:
TryMatchAggregateaccepts 1 or 2 arguments for predicate aggregates, so bothg.Any()andg.Any(predicate)match;Allhas no parameterless overload so it always takes the predicate path.RebuildLiftedAggregaterebuilds the call against the widened element type from the generic method definition, quoting the lambda forQueryableoverloads — no per-method handling.Testing
New specification tests (all providers, SQL Server baselines) mirroring the shapes #38668 added for the already-whitelisted aggregates:
GroupBy_Any_with_predicate_through_navigation_propertyGroupBy_Count_with_predicate_through_navigation_propertyGroupBy_All_with_predicate_through_navigation_propertyGroupBy_Queryable_Any_with_predicate_through_navigation_propertyAsQueryable()source pathGroupBy_Queryable_All_with_predicate_through_navigation_propertyGroupBy_Any_and_aggregate_through_navigation_propertyAny()alongside a lifted aggregateGroupBy_All_and_aggregate_through_navigation_propertyAllpredicate reading no navigation, alongside a lifted aggregateGroupBy_multiple_aggregates_with_Any_and_All_sharing_same_navigationGroupBy_multiple_aggregates_sharing_same_navigationGroupBy_Any_through_two_level_navigationGroupBy_aggregate_through_two_level_navigationGroupBy_key_and_Any_through_same_navigationGroupBy_key_and_aggregate_through_same_navigationGroupBy_Any_through_navigation_in_intermediate_projectionGroupBy_aggregate_through_navigation_in_intermediate_projectionGroupBy_Any_through_filtered_navigationGroupBy_aggregate_through_filtered_navigationGroupBy_Any_through_filtered_navigation_with_totalGroupBy_aggregate_through_filtered_navigation_with_totalGroupBy_Any_through_filtered_navigation_ignore_query_filtersGroupBy_aggregate_through_filtered_navigation_ignore_query_filters