-
Notifications
You must be signed in to change notification settings - Fork 3.4k
MinBy MaxBy support #37573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
MinBy MaxBy support #37573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for SQL translation of .NET 6's MinBy/MaxBy LINQ methods to Entity Framework Core, addressing issue #25566. The implementation lowers MinBy/MaxBy operations to OrderBy/OrderByDescending followed by First/FirstOrDefault, with special handling for complex selectors including anonymous types.
Changes:
- Added MinBy/MaxBy query method normalization that transforms them into OrderBy/OrderByDescending + First/FirstOrDefault patterns
- Implemented async extension methods (MinByAsync/MaxByAsync) for IQueryable
- Added comprehensive test coverage across SQL Server, InMemory, and Cosmos providers
- Included support for anonymous type selectors by expanding them into ThenBy/ThenByDescending chains
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs | Core implementation that transforms MinBy/MaxBy calls into OrderBy+First patterns with special handling for anonymous type selectors |
| src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs | Minor optimization to pre-allocate list capacity |
| src/EFCore/Extensions/EntityFrameworkQueryableExtensions.cs | Added MinByAsync and MaxByAsync extension methods with XML documentation |
| src/EFCore/Query/QueryableMethods.cs | Added MinBy and MaxBy MethodInfo properties |
| src/Shared/EnumerableMethods.cs | Added MinBy and MaxBy MethodInfo properties |
| test/EFCore.Specification.Tests/TestUtilities/QueryAsserter.cs | Added AssertMinBy and AssertMaxBy test assertion helpers (contains dead code issue) |
| test/EFCore.Specification.Tests/TestUtilities/ExpectedQueryRewritingVisitor.cs | Added query rewriting logic for expected results with anonymous type selectors |
| test/EFCore.Specification.Tests/Query/QueryTestBase.cs | Added AssertMinBy and AssertMaxBy helper methods |
| test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs | Added specification tests for MinBy/MaxBy in GroupBy scenarios and fixed #region formatting |
| test/EFCore.Specification.Tests/Query/NorthwindAggregateOperatorsQueryTestBase.cs | Added comprehensive specification tests for MinBy/MaxBy including edge cases |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs | Added SQL Server specific tests and SQL baselines (contains test method call bug) |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindAggregateOperatorsQuerySqlServerTest.cs | Added SQL Server specific tests and SQL baselines (contains test method call bug) |
| test/EFCore.InMemory.FunctionalTests/Query/NorthwindAggregateOperatorsQueryInMemoryTest.cs | Added InMemory provider specific tests |
| test/EFCore.Cosmos.FunctionalTests/Query/NorthwindAggregateOperatorsQueryCosmosTest.cs | Added Cosmos provider specific tests (contains test method call bug) |
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindAggregateOperatorsQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Cosmos.FunctionalTests/Query/NorthwindAggregateOperatorsQueryCosmosTest.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var keySelector = methodCallExpression.Arguments[1].UnwrapLambdaFromQuote(); | ||
|
|
||
| var firstMethod = sourceType.IsNullableValueType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@henriquewr can you explain the First() vs. FirstOrDefault() logic here, and why we wouldn't just always do FIrstOrDefault()? In other words, why the special case for nullable value types (and why it doesn't apply to reference types, which are also nullable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The First/OrDefault is selected just to mimic the behavior of LINQ:
For value types empty collections always throws:
new List<int>().MaxBy(x => x); // throws System.InvalidOperationException: 'Sequence contains no elements'
So to mimic the same behavior the First method is selected:
new List<int>().OrderBy(x => x).First(); // throws System.InvalidOperationException: 'Sequence contains no elements';
new List<int>().OrderBy(x => x).FirstOrDefault(); // returns null
For nullable value types:
new List<int?>().MaxBy(x => x); // returns null
For nullable types the method FirstOrDefault is selected:
new List<int?>().OrderBy(x => x).First(); // throws System.InvalidOperationException: 'Sequence contains no elements'
new List<int?>().OrderBy(x => x).FirstOrDefault(); // returns null
(and why it doesn't apply to reference types, which are also nullable)
Yep, I forgot about that, reference types returns null too for empty collections:
new List<Type/*just any reference type*/>().MaxBy(x => x); // returns null
fixes #25566
MinBy(x => x.Id) is lowered to OrderBy(x => x.Id).First/OrDefault()
MaxBy(x => x.Id) is lowered to OrderByDesceding(x => x.Id).First/OrDefault()
To comply with LINQ contract, the
Firstmethod is determined by the nullability of the source, if source is not nullable,Firstwould be selected, otherwiseFirstOrDefaultI followed the proposal of #25566 (comment)
Including implementing support for anonymous types in MaxBy/MinBy, that isn't supported in LINQ to Objects (Is supporting this really a great idea?)