Query: Expand navigations in GroupBy case

Resolves #15249
dotnet · Aug 21, 2019 · cb043a2 · cb043a2
1 parent 85a93ec
commit cb043a2
Show file tree

Hide file tree

Showing 10 changed files with 145 additions and 112 deletions.
diff --git a/...RelationalShapedQueryCompilingExpressionVisitor.CustomShaperCompilingExpressionVisitor.cs b/...RelationalShapedQueryCompilingExpressionVisitor.CustomShaperCompilingExpressionVisitor.cs
@@ -530,6 +530,11 @@ protected override Expression VisitExtension(Expression extensionExpression)
                         Expression.Constant(((LambdaExpression)Visit(collectionShaper.InnerShaper)).Compile()));
                 }
 
+                if (extensionExpression is GroupByShaperExpression)
+                {
+                    throw new InvalidOperationException("Client side GroupBy is not supported.");
+                }
+
                 return base.VisitExtension(extensionExpression);
             }
 

diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
@@ -1092,7 +1092,7 @@ private void AddJoin(
             }
 
             // Verify what are the cases of pushdown for inner & outer both sides
-            if (Limit != null || Offset != null || IsDistinct || GroupBy.Count > 1)
+            if (Limit != null || Offset != null || IsDistinct || GroupBy.Count > 0)
             {
                 var sqlRemappingVisitor = new SqlRemappingVisitor(PushdownIntoSubquery(), (SelectExpression)Tables[0]);
                 innerSelectExpression = sqlRemappingVisitor.Remap(innerSelectExpression);
@@ -1105,7 +1105,7 @@ private void AddJoin(
                 || innerSelectExpression.IsDistinct
                 || innerSelectExpression.Predicate != null
                 || innerSelectExpression.Tables.Count > 1
-                || innerSelectExpression.GroupBy.Count > 1)
+                || innerSelectExpression.GroupBy.Count > 0)
             {
                 joinPredicate = new SqlRemappingVisitor(
                     innerSelectExpression.PushdownIntoSubquery(), (SelectExpression)innerSelectExpression.Tables[0])

diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
@@ -893,46 +893,59 @@ private Expression ProcessGroupBy(
             LambdaExpression elementSelector,
             LambdaExpression resultSelector)
         {
-            source = (NavigationExpansionExpression)_pendingSelectorExpandingExpressionVisitor.Visit(source);
-            var queryable = Reduce(source);
+            var keySelectorBody = ExpandNavigationsInLambdaExpression(source, keySelector);
             Expression result;
             if (elementSelector == null)
             {
+                source = (NavigationExpansionExpression)_pendingSelectorExpandingExpressionVisitor.Visit(source);
+                // TODO: Flow include in future
+                //source = (NavigationExpansionExpression)new IncludeApplyingExpressionVisitor(
+                //    this, _queryCompilationContext.IsTracking).Visit(source);
+                keySelector = GenerateLambda(keySelectorBody, source.CurrentParameter);
+                elementSelector = GenerateLambda(source.PendingSelector, source.CurrentParameter);
                 if (resultSelector == null)
                 {
                     result = Expression.Call(
-                        QueryableMethods.GroupByWithKeySelector.MakeGenericMethod(
-                            queryable.Type.TryGetSequenceType(), keySelector.ReturnType),
-                        queryable,
-                        Expression.Quote(keySelector));
+                        QueryableMethods.GroupByWithKeyElementSelector.MakeGenericMethod(
+                            source.CurrentParameter.Type, keySelector.ReturnType, elementSelector.ReturnType),
+                        source.Source,
+                        Expression.Quote(keySelector),
+                        Expression.Quote(elementSelector));
                 }
                 else
                 {
                     result = Expression.Call(
-                        QueryableMethods.GroupByWithKeyResultSelector.MakeGenericMethod(
-                            queryable.Type.TryGetSequenceType(), keySelector.ReturnType, resultSelector.ReturnType),
-                        queryable,
+                        QueryableMethods.GroupByWithKeyElementResultSelector.MakeGenericMethod(
+                            source.CurrentParameter.Type, keySelector.ReturnType, elementSelector.ReturnType, resultSelector.ReturnType),
+                        source.Source,
                         Expression.Quote(keySelector),
+                        Expression.Quote(elementSelector),
                         Expression.Quote(resultSelector));
                 }
             }
             else
             {
+                source = (NavigationExpansionExpression)ProcessSelect(source, elementSelector);
+                source = (NavigationExpansionExpression)_pendingSelectorExpandingExpressionVisitor.Visit(source);
+                source = (NavigationExpansionExpression)new IncludeApplyingExpressionVisitor(
+                    this, _queryCompilationContext.IsTracking).Visit(source);
+                keySelector = GenerateLambda(keySelectorBody, source.CurrentParameter);
+                elementSelector = GenerateLambda(source.PendingSelector, source.CurrentParameter);
                 if (resultSelector == null)
                 {
                     result = Expression.Call(
                         QueryableMethods.GroupByWithKeyElementSelector.MakeGenericMethod(
-                            queryable.Type.TryGetSequenceType(), keySelector.ReturnType, elementSelector.ReturnType),
-                        queryable,
+                            source.CurrentParameter.Type, keySelector.ReturnType, elementSelector.ReturnType),
+                        source.Source,
                         Expression.Quote(keySelector),
                         Expression.Quote(elementSelector));
                 }
                 else
                 {
                     result = Expression.Call(
                         QueryableMethods.GroupByWithKeyElementResultSelector.MakeGenericMethod(
-                            queryable.Type.TryGetSequenceType(), keySelector.ReturnType, elementSelector.ReturnType, resultSelector.ReturnType),
-                        queryable,
+                            source.CurrentParameter.Type, keySelector.ReturnType, elementSelector.ReturnType, resultSelector.ReturnType),
+                        source.Source,
                         Expression.Quote(keySelector),
                         Expression.Quote(elementSelector),
                         Expression.Quote(resultSelector));

diff --git a/test/EFCore.Relational.Specification.Tests/Query/QueryNoClientEvalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/QueryNoClientEvalTestBase.cs
@@ -200,7 +200,7 @@ public virtual void Throws_when_group_join()
             }
         }
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual void Throws_when_group_by()
         {
             using (var context = CreateContext())

diff --git a/test/EFCore.Specification.Tests/Query/AsyncGearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/AsyncGearsOfWarQueryTestBase.cs
@@ -21,7 +21,7 @@ protected AsyncGearsOfWarQueryTestBase(TFixture fixture)
 
         protected GearsOfWarContext CreateContext() => Fixture.CreateContext();
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual async Task Include_with_group_by_on_entity_qsre()
         {
             using (var ctx = CreateContext())
@@ -39,7 +39,7 @@ public virtual async Task Include_with_group_by_on_entity_qsre()
             }
         }
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual async Task Include_with_group_by_on_entity_qsre_with_composite_key()
         {
             using (var ctx = CreateContext())
@@ -57,7 +57,7 @@ public virtual async Task Include_with_group_by_on_entity_qsre_with_composite_ke
             }
         }
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual async Task Include_with_group_by_on_entity_navigation()
         {
             using (var ctx = CreateContext())
@@ -75,7 +75,7 @@ public virtual async Task Include_with_group_by_on_entity_navigation()
             }
         }
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual async Task Include_groupby_constant()
         {
             using (var ctx = CreateContext())

diff --git a/test/EFCore.Specification.Tests/Query/AsyncSimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/AsyncSimpleQueryTestBase.cs
@@ -29,7 +29,7 @@ protected AsyncSimpleQueryTestBase(TFixture fixture)
 
         protected NorthwindContext CreateContext() => Fixture.CreateContext();
 
-        [ConditionalFact(Skip = "Issue #15249")]
+        [ConditionalFact(Skip = "Issue#17068")]
         public virtual async Task GroupBy_tracking_after_dispose()
         {
             List<IGrouping<string, Order>> groups;

diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
@@ -390,7 +390,7 @@ public virtual Task Simple_level1_level2_include(bool isAsync)
                 l1s => l1s.Include(l1 => l1.OneToOne_Required_PK1.OneToOne_Required_PK2), elementSorter: e => e.Id);
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Simple_level1_level2_GroupBy_Count(bool isAsync)
         {
@@ -408,7 +408,7 @@ public virtual Task Simple_level1_level2_GroupBy_Count(bool isAsync)
                     .Select(g => g.Count()));
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Simple_level1_level2_GroupBy_Having_Count(bool isAsync)
         {
@@ -1888,7 +1888,7 @@ where Maybe(l1.OneToOne_Optional_FK1, () => l1.OneToOne_Optional_FK1.Name) != "L
                 elementSorter: l1 => l1.Id);
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "issue #17068")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_with_groupjoin_skip_and_take(bool isAsync)
         {
@@ -5098,7 +5098,7 @@ public virtual void Entries_for_detached_entities_are_removed()
             }
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#12088")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_reference_with_groupby_in_subquery(bool isAsync)
         {
@@ -5114,7 +5114,7 @@ public virtual Task Include_reference_with_groupby_in_subquery(bool isAsync)
                 });
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#12088")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_collection_with_groupby_in_subquery(bool isAsync)
         {
@@ -5130,7 +5130,7 @@ public virtual Task Include_collection_with_groupby_in_subquery(bool isAsync)
                 });
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#12088")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Multi_include_with_groupby_in_subquery(bool isAsync)
         {
@@ -5149,7 +5149,7 @@ public virtual Task Multi_include_with_groupby_in_subquery(bool isAsync)
                 expectedIncludes);
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#12088")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_collection_with_groupby_in_subquery_and_filter_before_groupby(bool isAsync)
         {
@@ -5166,7 +5166,7 @@ public virtual Task Include_collection_with_groupby_in_subquery_and_filter_befor
                 });
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#12088")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_collection_with_groupby_in_subquery_and_filter_after_groupby(bool isAsync)
         {
@@ -5388,7 +5388,7 @@ public virtual Task Include_after_SelectMany_and_multiple_reference_navigations(
                 });
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "Issue#16752")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Include_after_SelectMany_and_reference_navigation_with_another_SelectMany_with_Distinct(bool isAsync)
         {

diff --git a/test/EFCore.Specification.Tests/Query/GroupByQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GroupByQueryTestBase.cs
@@ -37,13 +37,15 @@ public virtual Task GroupBy_Property_Select_Average(bool isAsync)
                 os => os.GroupBy(o => o.CustomerID).Select(g => g.Average(o => o.OrderID)));
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory(Skip = "issue #17068")]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Property_Select_Average_with_navigation_expansion(bool isAsync)
         {
             return AssertQueryScalar<Order>(
                 isAsync,
-                os => os.Where(o => o.Customer.City != "London").GroupBy(o => o.CustomerID, (k, es) => new { k, es }).Select(g => g.es.Average(o => o.OrderID)));
+                os => os.Where(o => o.Customer.City != "London")
+                    .GroupBy(o => o.CustomerID, (k, es) => new { k, es })
+                    .Select(g => g.es.Average(o => o.OrderID)));
         }
 
         [ConditionalTheory]
@@ -1452,7 +1454,7 @@ on o.CustomerID equals c.CustomerID
                 e => e.Key);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_required_navigation_member_Aggregate(bool isAsync)
         {
@@ -1602,7 +1604,7 @@ from c in grouping.DefaultIfEmpty()
                 e => e.Value);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_optional_navigation_member_Aggregate(bool isAsync)
         {
@@ -1664,7 +1666,7 @@ on o1.OrderID equals o2.OrderID
                 e => e.Key);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_multi_navigation_members_Aggregate(bool isAsync)
         {
@@ -1729,7 +1731,7 @@ public virtual Task Select_anonymous_GroupBy_Aggregate(bool isAsync)
                         }));
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_principal_key_property_optimization(bool isAsync)
         {
@@ -1930,7 +1932,7 @@ public virtual Task GroupBy_filter_count_OrderBy_count_Select_sum(bool isAsync)
                             }));
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Aggregate_Join(bool isAsync)
         {
@@ -1955,7 +1957,7 @@ join o in os on a.LastOrderID equals o.OrderID
                 entryCount: 126);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Join_GroupBy_Aggregate_multijoins(bool isAsync)
         {
@@ -1981,7 +1983,7 @@ join o in os on a.LastOrderID equals o.OrderID
                 entryCount: 126);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Join_GroupBy_Aggregate_single_join(bool isAsync)
         {
@@ -2006,7 +2008,7 @@ on c.CustomerID equals a.CustomerID
                 entryCount: 63);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Join_GroupBy_Aggregate_with_another_join(bool isAsync)
         {
@@ -2034,7 +2036,7 @@ from g in grouping
                 entryCount: 63);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Join_GroupBy_Aggregate_in_subquery(bool isAsync)
         {
@@ -2064,10 +2066,10 @@ on o.CustomerID equals i.c.CustomerID
                         i.c,
                         i.c.CustomerID
                     },
-                entryCount: 133);
+                entryCount: 187);
         }
 
-        [ConditionalTheory(Skip = "Issue#15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Join_GroupBy_Aggregate_on_key(bool isAsync)
         {