Skip to content

Keep conversions around a grouping key on the server - #39368

Merged
AndriySvyryd merged 1 commit into
dotnet:mainfrom
benedict-odonovan:fix/groupby-lift-key-conversion
Oct 23, 2026
Merged

AndriySvyryd merged 1 commit into
dotnet:mainfrom
benedict-odonovan:fix/groupby-lift-key-conversion

Conversation

@benedict-odonovan

Copy link
Copy Markdown
Contributor

Fixes #39367

  • 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

Summary

Since #38963, a projection that enumerates a grouping's elements is lifted. It's translated as a single scan, and the rest of the projection is applied on the client to each materialized grouping. That client selector accepted any conversion around g.Key, so (decimal)g.Key or (int)g.Key ran in .NET instead of as a SQL CAST, and the results changed:

Query Before #38963 / release/11.0 main
(decimal)g.Key over a double key 1.237 (SQL Server) 1.24 (CAST(... AS decimal(18,2))) 1.2370000000000000994759830064
(int)g.Key over a long key 3,000,000,000 SqlException: arithmetic overflow -1294967296
(int?)g.Key over a money key 2.5 (SQL Server) 3 (CAST rounds) 2 (.NET truncates)

The same key next to an aggregate, Select(g => new { Key = (decimal)g.Key, Count = g.Count() }), is still cast on the server, so two projections of the same key disagreed.

Change

GroupingElementProjectionAnalyzer.Visit now lets a conversion through to the client selector only when it can't change the value:

  • wrapping in or unwrapping from Nullable<T>;
  • boxing;
  • a reference conversion up the type hierarchy.

Any other conversion, including every conversion that has a Method (op_Explicit, op_Implicit), marks the projection as unsupported. It then keeps its non-lifted translation, with the CAST on the server as before.

This gives up the single scan only for projections that convert the key to a different type. The alternative was to translate each conversion and lift only when it translates to nothing, as #39240 does for a conversion around an aggregate. That would keep a few more projections lifted, but it needs the SQL translator inside the analyzer, which doesn't have it today.

Note on null keys. A projection that falls back to the non-lifted translation is subject to #32026: with a nullable key, the null group loses its elements. That's fixed separately in #39365. The two PRs are independent and touch different files.

Tests

  • GroupBy_selecting_grouping_element_list_with_converted_key: (int)g.Key over a short key is no longer lifted, so the SQL has CAST(... AS int) and GROUP BY. The conversion was chosen so that SQL and LINQ-to-Objects agree on the values, since the spec tests compare against LINQ-to-Objects; the baseline is what shows the server-side cast. (UnitPrice would show the rounding difference, but then the test's expected LINQ-to-Objects results would be the wrong ones.)
  • GroupBy_selecting_grouping_element_list_with_nullable_converted_key: (uint?)g.Key stays lifted (DENSE_RANK single scan).

Ran locally (LocalDB). All pass, with no other baseline changes:

Provider Test classes Tests
SQL Server *GroupBy*, *ComplexNavigations*, *GearsOfWar*, *Northwind*, *PrecompiledQuery* 13,466
SQLite same 11,530
InMemory same 7,652

Copilot AI balanced review requested due to automatic review settings October 22, 2026 13:31
@benedict-odonovan
benedict-odonovan requested a review from a team as a code owner October 22, 2026 13:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The reviewed changes address the conversion regression with targeted tests and provider-specific SQL verification.

No open issues 🎉

Review effort: Lite

@AndriySvyryd AndriySvyryd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@AndriySvyryd

Copy link
Copy Markdown
Member

@benedict-odonovan Please rebase on latest main

- A projection lifted out of a grouping ran a conversion around g.Key in its client selector, replacing the database's CAST
  semantics with .NET's: (decimal)g.Key over a double key no longer rounded to the mapped scale, and (int)g.Key over a long
  key wrapped around instead of failing on overflow.
- Only conversions which can't change the value - to or from the nullable form, boxing, and reference conversions - are now
  applied on the client. Any other conversion makes the projection keep its non-lifted translation, which casts on the server.
- Add tests for a key conversion which changes the value and one which doesn't.

Fixes dotnet#39367
Copilot AI lite review requested due to automatic review settings October 23, 2026 09:05
@benedict-odonovan
benedict-odonovan force-pushed the fix/groupby-lift-key-conversion branch from 21d6b49 to 26fe02c Compare October 23, 2026 09:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Add sync and async result-selector coverage for both rejected numeric and accepted nullable conversions, including the provider SQL baseline.

No open issues 🎉

Review effort: Lite

@AndriySvyryd
AndriySvyryd merged commit c0f183b into dotnet:main Oct 23, 2026
19 checks passed
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Oct 24, 2026
@benedict-odonovan
benedict-odonovan deleted the fix/groupby-lift-key-conversion branch October 26, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GroupBy element projection evaluates a conversion around g.Key on the client instead of as a SQL CAST

3 participants