Fix to dotnet#6861 - Query with projection and navigation properties …

…adds extra sort columns in ORDER BY for foreign key columns When creating LEFT OUTER JOIN we add ORDER BY clause(s) based on the key of the outer entity. This is because out GroupJoin method is streaming and we need to have results in order to create the correct groups in a performant way. However we can there are some optimizations we can do: 1. we don't need ORDER BY if the join is from dependent to principal because there will always be at most one customer associated with a given order. e.g. orders.Select(o => o.Customer.Name) which translates to: from o in Orders join c in Customers on o.CustomerId equal c.Id into grouping from c in grouping.DefaultIfEmpty() select c.Name In case of composite keys we need to check that entire key is being used as a join condition. Moreover, if there are additional elements in the join condition (on top of the FK->PK) we don't need to add order by either, because the grouping will be even more restrictive. 2. in case when we fully translate LOJ into SQL (which is now very often the case) we also don't need the extra ORDER BYs because we no longer perform streamed grouping on the client.
davidMuir · Nov 21, 2016 · 398dec4 · 398dec4
1 parent de109a3
commit 398dec4
Show file tree

Hide file tree

Showing 8 changed files with 474 additions and 383 deletions.
diff --git a/src/Microsoft.EntityFrameworkCore.Relational/Query/Expressions/SelectExpression.cs b/src/Microsoft.EntityFrameworkCore.Relational/Query/Expressions/SelectExpression.cs
@@ -912,6 +912,18 @@ public virtual void PrependToOrderBy([NotNull] IEnumerable<Ordering> orderings)
         /// </summary>
         public virtual void ClearOrderBy() => _orderBy.Clear();
 
+        /// <summary>
+        ///     Removes a range from list of order by elements.
+        /// </summary>
+        /// <param name="index"> Zero-based index of the start of the range to remove. </param>
+        public virtual void RemoveRangeFromOrderBy(int index)
+        {
+            if (index < _orderBy.Count)
+            {
+                _orderBy.RemoveRange(index, _orderBy.Count - index);
+            }
+        }
+
         /// <summary>
         ///     Transforms the projection of this SelectExpression by expanding the wildcard ('*') projection
         ///     into individual explicit projection expressions.

diff --git a/src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs b/src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs
@@ -711,13 +711,16 @@ var joinExpression
                         if (groupJoin)
                         {
                             var outerJoinOrderingExtractor = new OuterJoinOrderingExtractor();
-
                             outerJoinOrderingExtractor.Visit(predicate);
 
-                            foreach (var expression in outerJoinOrderingExtractor.Expressions)
+                            var previousOrderingCount = previousSelectExpression.OrderBy.Count;
+                            if (!outerJoinOrderingExtractor.DependentToPrincipalFound)
                             {
-                                previousSelectExpression
-                                    .AddToOrderBy(new Ordering(expression, OrderingDirection.Asc));
+                                foreach (var expression in outerJoinOrderingExtractor.Expressions)
+                                {
+                                    previousSelectExpression.AddToOrderBy(
+                                        new Ordering(expression, OrderingDirection.Asc));
+                                }
                             }
 
                             var additionalFromClause
@@ -784,6 +787,11 @@ var groupJoinClause
                                     QueriesBySource.Remove(joinClause);
 
                                     operatorToFlatten = LinqOperatorProvider.Join;
+
+                                    if (previousOrderingCount != previousSelectExpression.OrderBy.Count)
+                                    {
+                                        previousSelectExpression.RemoveRangeFromOrderBy(previousOrderingCount);
+                                    }
                                 }
                             }
                         }
@@ -861,25 +869,76 @@ private class OuterJoinOrderingExtractor : ExpressionVisitor
         {
             private readonly List<Expression> _expressions = new List<Expression>();
 
+            public bool DependentToPrincipalFound { get; private set; }
+
             public IEnumerable<Expression> Expressions => _expressions;
 
+            private IForeignKey _matchingCandidate;
+            private List<IProperty> _matchingCandidateProperties;
+
             public override Expression Visit(Expression expression)
             {
                 var binaryExpression = expression as BinaryExpression;
 
                 if (binaryExpression != null)
                 {
-                    switch (binaryExpression.NodeType)
+                    return VisitBinary(binaryExpression);
+                }
+
+                return expression;
+            }
+
+            protected override Expression VisitBinary(BinaryExpression node)
+            {
+                if (DependentToPrincipalFound)
+                {
+                    return node;
+                }
+
+                if (node.NodeType == ExpressionType.Equal)
+                {
+                    var leftProperty = node.Left.RemoveConvert().TryGetColumnExpression()?.Property;
+                    var rightProperty = node.Right.RemoveConvert().TryGetColumnExpression()?.Property;
+                    if (leftProperty != null && rightProperty != null && leftProperty.IsForeignKey() && rightProperty.IsKey())
                     {
-                        case ExpressionType.Equal:
-                            _expressions.Add(binaryExpression.Left.RemoveConvert());
-                            return expression;
-                        case ExpressionType.AndAlso:
-                            return VisitBinary(binaryExpression);
+                        var keyDeclaringEntityType = rightProperty.GetContainingKeys().First().DeclaringEntityType;
+                        var matchingForeignKeys = leftProperty.GetContainingForeignKeys().Where(k => k.PrincipalKey.DeclaringEntityType == keyDeclaringEntityType);
+                        if (matchingForeignKeys.Count() == 1)
+                        {
+                            var matchingKey = matchingForeignKeys.Single();
+                            if (rightProperty.GetContainingKeys().Contains(matchingKey.PrincipalKey))
+                            {
+                                var matchingForeignKey = matchingKey;
+                                if (_matchingCandidate == null)
+                                {
+                                    _matchingCandidate = matchingForeignKey;
+                                    _matchingCandidateProperties = new List<IProperty> { leftProperty };
+                                }
+                                else if (_matchingCandidate == matchingForeignKey)
+                                {
+                                    _matchingCandidateProperties.Add(leftProperty);
+                                }
+
+                                if (_matchingCandidate.Properties.All(p => _matchingCandidateProperties.Contains(p)))
+                                {
+                                    DependentToPrincipalFound = true;
+                                    return node;
+                                }
+                            }
+                        }
                     }
+
+                    _expressions.Add(node.Left.RemoveConvert());
+
+                    return node;
                 }
 
-                return expression;
+                if (node.NodeType == ExpressionType.AndAlso)
+                {
+                    return base.VisitBinary(node);
+                }
+
+                return node;
             }
         }
 

diff --git a/src/Microsoft.EntityFrameworkCore.Specification.Tests/GearsOfWarQueryTestBase.cs b/src/Microsoft.EntityFrameworkCore.Specification.Tests/GearsOfWarQueryTestBase.cs
@@ -2078,6 +2078,27 @@ public virtual void DateTimeOffset_Datepart_works()
             }
         }
 
+        [ConditionalFact]
+        public virtual void Orderby_added_for_client_side_GroupJoin_composite_dependent_to_principal_LOJ_when_incomplete_key_is_used()
+        {
+            using (var ctx = CreateContext())
+            {
+                var query = from t in ctx.Tags
+                            join g in ctx.Gears on t.GearNickName equals g.Nickname into grouping
+                            from g in ClientDefaultIfEmpty(grouping)
+                            select new { Note = t.Note, Nickname = (g != null ? g.Nickname : null) };
+
+                var result = query.ToList();
+
+                Assert.Equal(6, result.Count);
+            }
+        }
+
+        private static IEnumerable<TElement> ClientDefaultIfEmpty<TElement>(IEnumerable<TElement> source)
+        {
+            return source?.Count() == 0 ? new[] { default(TElement) } : source;
+        }
+
         protected GearsOfWarContext CreateContext() => Fixture.CreateContext(TestStore);
 
         protected GearsOfWarQueryTestBase(TFixture fixture)

diff --git a/src/Microsoft.EntityFrameworkCore.Specification.Tests/QueryTestBase.cs b/src/Microsoft.EntityFrameworkCore.Specification.Tests/QueryTestBase.cs
@@ -6470,6 +6470,60 @@ public virtual void OrderBy_skip_take_level_3()
                 entryCount: 5);
         }
 
+        [ConditionalFact]
+        public virtual void No_orderby_added_for_fully_translated_manually_constructed_LOJ()
+        {
+            AssertQuery<Employee>(
+                es => from e1 in es
+                      join e2 in es on e1.EmployeeID equals e2.ReportsTo into grouping
+                      from e2 in grouping.DefaultIfEmpty()
+                      select new { City1 = e1.City, City2 = e2 != null ? e2.City : null });
+        }
+
+        [ConditionalFact]
+        public virtual void No_orderby_added_for_client_side_GroupJoin_dependent_to_principal_LOJ()
+        {
+            AssertQuery<Customer, Order>(
+                (cs, os) => from o in os
+                            join c in cs on o.CustomerID equals c.CustomerID into grouping
+                            from c in ClientDefaultIfEmpty(grouping)
+                            select new { Id1 = o.CustomerID, Id2 = c != null ? c.CustomerID : null });
+        }
+
+        [ConditionalFact]
+        public virtual void No_orderby_added_for_client_side_GroupJoin_dependent_to_principal_LOJ_with_additional_join_condition1()
+        {
+            AssertQuery<Customer, Order>(
+                (cs, os) => from o in os
+                            join c in cs on new { o.CustomerID, o.OrderID } equals new { c.CustomerID, OrderID = 10000 } into grouping
+                            from c in ClientDefaultIfEmpty(grouping)
+                            select new { Id1 = o.CustomerID, Id2 = c != null ? c.CustomerID : null });
+        }
+
+        [ConditionalFact]
+        public virtual void No_orderby_added_for_client_side_GroupJoin_dependent_to_principal_LOJ_with_additional_join_condition2()
+        {
+            AssertQuery<Customer, Order>(
+                (cs, os) => from o in os
+                            join c in cs on new { o.OrderID, o.CustomerID } equals new { OrderID = 10000, c.CustomerID,  } into grouping
+                            from c in ClientDefaultIfEmpty(grouping)
+                            select new { Id1 = o.CustomerID, Id2 = c != null ? c.CustomerID : null });
+        }
+
+        [ConditionalFact]
+        public virtual void Orderby_added_for_client_side_GroupJoin_principal_to_dependent_LOJ()
+        {
+            AssertQuery<Employee>(
+                es => from e1 in es
+                      join e2 in es on e1.EmployeeID equals e2.ReportsTo into grouping
+                      from e2 in ClientDefaultIfEmpty(grouping)
+                      select new { City1 = e1.City, City2 = e2 != null ? e2.City : null });
+        }
+
+        private static IEnumerable<TElement> ClientDefaultIfEmpty<TElement>(IEnumerable<TElement> source)
+        {
+            return source?.Count() == 0 ? new[] { default(TElement) } : source;
+        }
 
         protected NorthwindContext CreateContext() => Fixture.CreateContext();