Repository navigation
Fix MinBy/MaxBy null-key handling with index-friendly selection - #39653
Open
emremenekse wants to merge 4 commits into
Open
emremenekse wants to merge 4 commits into
emremenekse wants to merge 4 commits into
Conversation
- Order non-null keys first and retain explicit source ordering for ties. - Preserve empty and all-null source behavior. - Add shared regression coverage for SQLite, SQL Server, and InMemory. Fixes dotnet#39276
- Find extremal keys separately while preserving source order and all-null fallback. - Use guarded SQLite IS translation to avoid the candidate union and final sort. - Cover ties, pagination, empty sources, relational nulls and precompiled queries. Fixes dotnet#39276
- Override the shared nullable-key test and assert all five SQL commands. - Satisfy the existing check that requires provider-specific test overrides. Fixes dotnet#39276
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.
MinBy/MaxBy currently lower to ordering followed by First/FirstOrDefault. Database null ordering can select a null-key row even when non-null keys exist, unlike LINQ-to-Objects. For example, MinBy over keys [null, 1, 2] can select the null-key row instead of the row with key 1.
Prefer non-null keys while retaining the first source row for an all-null source and preserving empty-source behavior. For eligible plain entity queries, find the extremal key separately from source ordering, then select its first occurrence. The general lowering remains available for unsupported candidate shapes, including owned/JSON entities, scalar sources and computed selectors.
Add a SQLite-specific translation for the recognized two-candidate lowering. Match the source, key and partition predicates, then replace the candidate union with a null-safe SQLite IS comparison against the extremal-key subquery. This removes the fallback candidate and final sort for the optimized shape. Decline the rewrite for different sources, unrecognized method calls, explicit volatile expressions and view/function/raw-SQL roots. SQL Server continues to use the common candidate lowering; the IS optimization is confined to SQLite. The new SQL expression supports quoting for precompiled queries and is non-nullable even when either operand is nullable.
Fixes #39276.
Validation
Targeted local tests on Windows:
Coverage includes integer/string nullable keys, sync/async, mixed-null keys, all-null keys, an empty table, tied keys, reverse ordering, pagination and Id=0. SQLite SQL baselines assert the new translation. Separate tests verify that different candidate/key sources and volatile sources retain the general translation, and that all-null behavior is preserved with UseRelationalNulls.
Precompiled MinByAsync/MaxByAsync recognition remains disabled in the existing query locator/code generator. This revision tests synchronous precompiled calls and normal asynchronous execution.
Local performance checks
Repeated the original 32 Release comparisons: SQLite/SQL Server, 1,000/100,000-row ranges, indexed/unindexed, int/int? keys, MinBy/MaxBy. The short run showed substantial timing noise even for identical-SQL controls, so its raw alarms were retained and the critical indexed cases were rechecked with longer batches.
A second run used 100 warmup calls and seven alternating batches of 1,000 EF operations for each of four indexed 100,000-row comparisons. None crossed the unchanged local >=2x diagnostic threshold. This is a diagnostic threshold, not evidence of zero overhead:
The simple reference returns the wrong row for mixed-null MinBy. Both MaxBy versions return the same row in these cases. The measurements show remaining overhead compared with that simple reference, not an end-to-end speedup.
The previous correct candidate SQL and the new SQLite SQL were also executed on the same database with fresh ADO.NET commands and identical parameters. Both were required to return the same expected record. With 100 warmup calls and seven alternating batches of 500 operations:
This approximately 70-75% reduction covers SQL command creation/preparation/execution and reading the selected ID, not the full EF pipeline or application throughput. These are local medians, not BenchmarkDotNet confidence intervals or production guarantees.
The measured SQLite indexed plans use two covering-index searches without UNION ALL or a temporary sorting B-tree. All 32 captured SQL Server SQL files were byte-identical before and after adding the SQLite-specific optimization; no SQL Server performance improvement is claimed.
Limits
Contribution checklist
Local targeted validation passed as described above. CI, the walkthrough and team approval have not been verified.