Query: Final GroupBy with non-nullable column

Resolves #19929
Resolves #28300

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

@@ -867,7 +867,7 @@ public Expression ApplyProjection(
             if (shaperExpression is RelationalGroupByShaperExpression groupByShaper)
             {
                 // We need to add key to projection and generate key selector in terms of projectionBindings
-                var projectionBindingMap = new Dictionary<SqlExpression, ProjectionBindingExpression>();
+                var projectionBindingMap = new Dictionary<SqlExpression, Expression>();
                 var keySelector = AddGroupByKeySelectorToProjection(
                     this, newClientProjections, projectionBindingMap, groupByShaper.KeySelector);
                 var (keyIdentifier, keyIdentifierValueComparers) = GetIdentifierAccessor(
@@ -879,7 +879,7 @@ public Expression ApplyProjection(
                 Expression AddGroupByKeySelectorToProjection(
                     SelectExpression selectExpression,
                     List<Expression> clientProjectionList,
-                    Dictionary<SqlExpression, ProjectionBindingExpression> projectionBindingMap,
+                    Dictionary<SqlExpression, Expression> projectionBindingMap,
                     Expression keySelector)
                 {
                     switch (keySelector)
@@ -896,8 +896,11 @@ Expression AddGroupByKeySelectorToProjection(
                                 existingIndex = clientProjectionList.Count - 1;
                             }
 
-                            var projectionBindingExpression = new ProjectionBindingExpression(
-                                selectExpression, existingIndex, sqlExpression.Type.MakeNullable());
+                            var projectionBindingExpression = sqlExpression.Type.IsNullableType()
+                                ? (Expression)new ProjectionBindingExpression(selectExpression, existingIndex, sqlExpression.Type)
+                                : Convert(new ProjectionBindingExpression(
+                                    selectExpression, existingIndex, sqlExpression.Type.MakeNullable()),
+                                    sqlExpression.Type);
                             projectionBindingMap[sqlExpression] = projectionBindingExpression;
                             return projectionBindingExpression;
                         }
@@ -964,14 +967,14 @@ Expression AddGroupByKeySelectorToProjection(
                 static (Expression, IReadOnlyList<ValueComparer>) GetIdentifierAccessor(
                     SelectExpression selectExpression,
                     List<Expression> clientProjectionList,
-                    Dictionary<SqlExpression, ProjectionBindingExpression> projectionBindingMap,
+                    Dictionary<SqlExpression, Expression> projectionBindingMap,
                     IEnumerable<(ColumnExpression Column, ValueComparer Comparer)> identifyingProjection)
                 {
                     var updatedExpressions = new List<Expression>();
                     var comparers = new List<ValueComparer>();
                     foreach (var (column, comparer) in identifyingProjection)
                     {
-                        if (!projectionBindingMap.TryGetValue(column, out var projectionBindingExpression))
+                        if (!projectionBindingMap.TryGetValue(column, out var mappedExpresssion))
                         {
                             var index = selectExpression.AddToProjection(column);
                             var clientProjectionToAdd = Constant(index);
@@ -983,14 +986,13 @@ Expression AddGroupByKeySelectorToProjection(
                                 existingIndex = clientProjectionList.Count - 1;
                             }
 
-                            projectionBindingExpression = new ProjectionBindingExpression(
-                                selectExpression, existingIndex, column.Type.MakeNullable());
+                            mappedExpresssion = new ProjectionBindingExpression(selectExpression, existingIndex, column.Type.MakeNullable());
                         }
 
                         updatedExpressions.Add(
-                            projectionBindingExpression.Type.IsValueType
-                                ? Convert(projectionBindingExpression, typeof(object))
-                                : projectionBindingExpression);
+                            mappedExpresssion.Type.IsValueType
+                                ? Convert(mappedExpresssion, typeof(object))
+                                : mappedExpresssion);
                         comparers.Add(comparer);
                     }
 

test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs

@@ -118,6 +118,13 @@ public override void Value_conversion_on_enum_collection_contains()
             CoreStrings.TranslationFailed("")[47..],
             Assert.Throws<InvalidOperationException>(() => base.Value_conversion_on_enum_collection_contains()).Message);
 
+    public override void GroupBy_converted_enum()
+    {
+        Assert.Contains(
+            CoreStrings.TranslationFailed("")[21..],
+            Assert.Throws<InvalidOperationException>(() => base.GroupBy_converted_enum()).Message);
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.InMemory.FunctionalTests/CustomConvertersInMemoryTest.cs

@@ -1,6 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using Microsoft.EntityFrameworkCore.InMemory.Internal;
+using static Microsoft.EntityFrameworkCore.DbLoggerCategory;
+
 namespace Microsoft.EntityFrameworkCore;
 
 public class CustomConvertersInMemoryTest : CustomConvertersTestBase<CustomConvertersInMemoryTest.CustomConvertersInMemoryFixture>
@@ -36,6 +39,13 @@ public override void Collection_property_as_scalar_Count_member()
     public override void Collection_enum_as_string_Contains()
         => base.Collection_enum_as_string_Contains();
 
+    public override void GroupBy_converted_enum()
+    {
+        Assert.Contains(
+            CoreStrings.TranslationFailedWithDetails("", InMemoryStrings.NonComposedGroupByNotSupported)[21..],
+            Assert.Throws<InvalidOperationException>(() => base.GroupBy_converted_enum()).Message);
+    }
+
     public class CustomConvertersInMemoryFixture : CustomConvertersFixtureBase
     {
         public override bool StrictEquality

test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs

@@ -25,6 +25,11 @@ public override Task Final_GroupBy_entity(bool async)
             () => base.Final_GroupBy_entity(async),
             InMemoryStrings.NonComposedGroupByNotSupported);
 
+    public override Task Final_GroupBy_property_entity_non_nullable(bool async)
+       => AssertTranslationFailedWithDetails(
+           () => base.Final_GroupBy_property_entity_non_nullable(async),
+           InMemoryStrings.NonComposedGroupByNotSupported);
+
     public override Task Final_GroupBy_property_anonymous_type(bool async)
         => AssertTranslationFailedWithDetails(
             () => base.Final_GroupBy_property_anonymous_type(async),

test/EFCore.Specification.Tests/CustomConvertersTestBase.cs

@@ -811,6 +811,36 @@ public enum HoldingEnum
         Value2
     }
 
+    [ConditionalFact]
+    public virtual void GroupBy_converted_enum()
+    {
+        using var context = CreateContext();
+        var result = context.Set<Entity>().GroupBy(e => e.SomeEnum).ToList();
+
+        Assert.Collection(result,
+            t =>
+            {
+                Assert.Equal(SomeEnum.No, t.Key);
+                Assert.Single(t);
+            },
+            t =>
+            {
+                Assert.Equal(SomeEnum.Yes, t.Key);
+                Assert.Equal(2, t.Count());
+            });
+    }
+
+    public class Entity
+    {
+        public int Id { get; set; }
+        public SomeEnum SomeEnum { get; set; }
+    }
+    public enum SomeEnum
+    {
+        Yes,
+        No
+    }
+
     public abstract class CustomConvertersFixtureBase : BuiltInDataTypesFixtureBase
     {
         protected override string StoreName
@@ -1340,6 +1370,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                         v => new List<Layout>(v)));
 
             modelBuilder.Entity<HolderClass>().HasData(new HolderClass { Id = 1, HoldingEnum = HoldingEnum.Value2 });
+
+            modelBuilder.Entity<Entity>().Property(e => e.SomeEnum).HasConversion(e => e.ToString(), e => Enum.Parse<SomeEnum>(e));
+            modelBuilder.Entity<Entity>().HasData(
+                new Entity { Id = 1, SomeEnum = SomeEnum.Yes },
+                new Entity { Id = 2, SomeEnum = SomeEnum.No },
+                new Entity { Id = 3, SomeEnum = SomeEnum.Yes });
         }
 
         private static class StringToDictionarySerializer
@@ -1376,7 +1412,8 @@ public static List<Layout> Deserialize(string s)
                     list.Add(
                         new Layout
                         {
-                            Height = int.Parse(parts[0]), Width = int.Parse(parts[1]),
+                            Height = int.Parse(parts[0]),
+                            Width = int.Parse(parts[1]),
                         });
                 }
 

test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs

@@ -2606,6 +2606,16 @@ public virtual Task Final_GroupBy_entity(bool async)
             elementAsserter: (e, a) => AssertGrouping(e, a),
             entryCount: 328);
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Final_GroupBy_property_entity_non_nullable(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<OrderDetail>().Where(e => e.OrderID < 10500).GroupBy(c => c.OrderID),
+            elementSorter: e => e.Key,
+            elementAsserter: (e, a) => AssertGrouping(e, a),
+            entryCount: 664);
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Final_GroupBy_property_anonymous_type(bool async)

test/EFCore.SqlServer.FunctionalTests/CustomConvertersSqlServerTest.cs

@@ -159,6 +159,8 @@ public virtual void Columns_have_expected_data_types()
 DateTimeEnclosure.Id ---> [int] [Precision = 10 Scale = 0]
 EmailTemplate.Id ---> [uniqueidentifier]
 EmailTemplate.TemplateType ---> [int] [Precision = 10 Scale = 0]
+Entity.Id ---> [int] [Precision = 10 Scale = 0]
+Entity.SomeEnum ---> [nvarchar] [MaxLength = -1]
 EntityWithValueWrapper.Id ---> [int] [Precision = 10 Scale = 0]
 EntityWithValueWrapper.Wrapper ---> [nullable nvarchar] [MaxLength = -1]
 HolderClass.HoldingEnum ---> [int] [Precision = 10 Scale = 0]

test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs

@@ -3154,6 +3154,17 @@ WHERE [o].[OrderID] < 10500
 ORDER BY [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]");
     }
 
+    public override async Task Final_GroupBy_property_entity_non_nullable(bool async)
+    {
+        await base.Final_GroupBy_property_entity_non_nullable(async);
+
+        AssertSql(
+            @"SELECT [o].[OrderID], [o].[ProductID], [o].[Discount], [o].[Quantity], [o].[UnitPrice]
+FROM [Order Details] AS [o]
+WHERE [o].[OrderID] < 10500
+ORDER BY [o].[OrderID]");
+    }
+
     public override async Task Final_GroupBy_property_anonymous_type(bool async)
     {
         await base.Final_GroupBy_property_anonymous_type(async);