Skip to content

Commit

Permalink
Remove last ORDER BY for collection joins (#24360)
Browse files Browse the repository at this point in the history 
* Remove last ORDER BY for collection joins

Closes #19828

* Review comments

* Revert "Review comments"

This reverts commit 5a37a7c.

* Refactor pending ordering to be self-contained in ApplyProjection

* Address review comments

* Address review comments
  • Loading branch information
@roji
roji authored Aug 12, 2021
1 parent 9eb7116 commit 5bcdd2e
Show file tree
Hide file tree
Showing 42 changed files with 789 additions and 766 deletions.
33 changes: 27 additions & 6 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -728,11 +728,29 @@ static Expression RemoveConvert(Expression expression)
.Except(innerSelectExpression._childIdentifiers, _identifierComparer)
.Select(e => (e.Column.MakeNullable(), e.Comparer)));

foreach (var identifier in innerSelectExpression._identifier)
OrderingExpression? pendingOrdering = null;
foreach (var (identifierColumn, identifierComparer) in innerSelectExpression._identifier)
{
var updatedColumn = identifier.Column.MakeNullable();
_childIdentifiers.Add((updatedColumn, identifier.Comparer));
AppendOrdering(new OrderingExpression(updatedColumn, ascending: true));
var updatedColumn = identifierColumn.MakeNullable();
_childIdentifiers.Add((updatedColumn, identifierComparer));

// We omit the last ordering as an optimization
var orderingExpression = new OrderingExpression(updatedColumn, ascending: true);

if (!_orderings.Any(o => o.Expression.Equals(updatedColumn)))
{
if (pendingOrdering is not null)
{
if (orderingExpression.Equals(pendingOrdering))
{
continue;
}

AppendOrderingInternal(pendingOrdering);
}

pendingOrdering = orderingExpression;
}
}

var result = new SingleCollectionInfo(
Expand Down Expand Up @@ -1209,12 +1227,15 @@ public void AppendOrdering(OrderingExpression orderingExpression)
{
Check.NotNull(orderingExpression, nameof(orderingExpression));

if (_orderings.FirstOrDefault(o => o.Expression.Equals(orderingExpression.Expression)) == null)
if (!_orderings.Any(o => o.Expression.Equals(orderingExpression.Expression)))
{
_orderings.Add(orderingExpression.Update(AssignUniqueAliases(orderingExpression.Expression)));
AppendOrderingInternal(orderingExpression);
}
}

private void AppendOrderingInternal(OrderingExpression orderingExpression)
=> _orderings.Add(orderingExpression.Update(AssignUniqueAliases(orderingExpression.Expression)));

/// <summary>
/// Reverses the existing orderings on the <see cref="SelectExpression" />.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6568,7 +6568,7 @@ public virtual Task Nullable_bool_comparison_is_translated_to_server(bool async)

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Acessing_reference_navigation_collection_composition_generates_single_query(bool async)
public virtual Task Accessing_reference_navigation_collection_composition_generates_single_query(bool async)
{
return AssertQuery(
async,
Expand Down
10 changes: 6 additions & 4 deletions test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1095,11 +1095,13 @@ public IReadOnlyDictionary<Type, object> GetEntityAsserters()
Assert.Equal(ee.Id, aa.Id);
Assert.Equal(ee.Name, aa.Name);
Assert.Equal(ee.Composition.Count, aa.Composition.Count);
for (var i = 0; i < ee.Composition.Count; i++)
foreach (var (eec, aac) in Enumerable.Zip(
ee.Composition.OrderBy(eec => eec.Id),
aa.Composition.OrderBy(aac => aac.Id)))
{
Assert.Equal(ee.Composition[i].Id, aa.Composition[i].Id);
Assert.Equal(ee.Composition[i].Name, aa.Composition[i].Name);
Assert.Equal(ee.Composition[i].StarId, aa.Composition[i].StarId);
Assert.Equal(eec.Id, aac.Id);
Assert.Equal(eec.Name, aac.Name);
Assert.Equal(eec.StarId, aac.StarId);
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions test/EFCore.SqlServer.FunctionalTests/ManyToManyFieldsLoadSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ FROM [JoinOneToTwo] AS [j0]
WHERE [e1].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[OneId], [t].[TwoId], [t].[Id], [t0].[OneId], [t0].[TwoId], [t0].[Id]");
ORDER BY [e].[Id], [t].[OneId], [t].[TwoId], [t].[Id], [t0].[OneId], [t0].[TwoId]");
}

public override async Task Load_collection_using_Query_with_Include_for_inverse(bool async)
Expand All @@ -64,7 +64,7 @@ FROM [EntityOneEntityTwo] AS [e2]
WHERE [e3].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId]");
}

public override async Task Load_collection_using_Query_with_Include_for_same_collection(bool async)
Expand Down Expand Up @@ -93,7 +93,7 @@ FROM [EntityOneEntityTwo] AS [e4]
WHERE [e3].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t0].[EntityOneId0], [t0].[EntityTwoId0], [t0].[Id0]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t0].[EntityOneId0], [t0].[EntityTwoId0]");
}

public override async Task Load_collection_using_Query_with_Include(bool async)
Expand Down Expand Up @@ -122,7 +122,7 @@ FROM [JoinTwoToThree] AS [j]
INNER JOIN [EntityThrees] AS [e4] ON [j].[ThreeId] = [e4].[Id]
) AS [t1] ON [t].[Id] = [t1].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId], [t1].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId]");
}

public override async Task Load_collection_using_Query_with_filtered_Include(bool async)
Expand Down Expand Up @@ -152,7 +152,7 @@ FROM [JoinTwoToThree] AS [j]
WHERE [e4].[Id] IN (13, 11)
) AS [t1] ON [t].[Id] = [t1].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId], [t1].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId]");
}

public override async Task Load_collection_using_Query_with_filtered_Include_and_projection(bool async)
Expand Down Expand Up @@ -211,7 +211,7 @@ FROM [EntityOneEntityTwo] AS [e5]
WHERE [e6].[Id] = @__p_0
) AS [t2] ON [t].[Id] = [t2].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id0], [t2].[EntityOneId], [t2].[EntityTwoId], [t2].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id0], [t2].[EntityOneId], [t2].[EntityTwoId]");
}

protected override void ClearLog()
Expand Down
12 changes: 6 additions & 6 deletions test/EFCore.SqlServer.FunctionalTests/ManyToManyLoadSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ FROM [JoinOneToTwo] AS [j0]
WHERE [e1].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[OneId], [t].[TwoId], [t].[Id], [t0].[OneId], [t0].[TwoId], [t0].[Id]");
ORDER BY [e].[Id], [t].[OneId], [t].[TwoId], [t].[Id], [t0].[OneId], [t0].[TwoId]");
}

public override async Task Load_collection_using_Query_with_Include_for_inverse(bool async)
Expand All @@ -63,7 +63,7 @@ FROM [EntityOneEntityTwo] AS [e2]
WHERE [e3].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId]");
}

public override async Task Load_collection_using_Query_with_Include_for_same_collection(bool async)
Expand Down Expand Up @@ -92,7 +92,7 @@ FROM [EntityOneEntityTwo] AS [e4]
WHERE [e3].[Id] = @__p_0
) AS [t0] ON [t].[Id] = [t0].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t0].[EntityOneId0], [t0].[EntityTwoId0], [t0].[Id0]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t0].[EntityOneId0], [t0].[EntityTwoId0]");
}

public override async Task Load_collection_using_Query_with_Include(bool async)
Expand Down Expand Up @@ -121,7 +121,7 @@ FROM [JoinTwoToThree] AS [j]
INNER JOIN [EntityThrees] AS [e4] ON [j].[ThreeId] = [e4].[Id]
) AS [t1] ON [t].[Id] = [t1].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId], [t1].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId]");
}

public override async Task Load_collection_using_Query_with_filtered_Include(bool async)
Expand Down Expand Up @@ -151,7 +151,7 @@ FROM [JoinTwoToThree] AS [j]
WHERE [e4].[Id] IN (13, 11)
) AS [t1] ON [t].[Id] = [t1].[TwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId], [t1].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id], [t1].[ThreeId], [t1].[TwoId]");
}

public override async Task Load_collection_using_Query_with_filtered_Include_and_projection(bool async)
Expand Down Expand Up @@ -210,7 +210,7 @@ FROM [EntityOneEntityTwo] AS [e5]
WHERE [e6].[Id] = @__p_0
) AS [t2] ON [t].[Id] = [t2].[EntityTwoId]
WHERE [e].[Id] = @__p_0
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id0], [t2].[EntityOneId], [t2].[EntityTwoId], [t2].[Id]");
ORDER BY [e].[Id], [t].[EntityOneId], [t].[EntityTwoId], [t].[Id], [t0].[Id], [t0].[EntityOneId], [t0].[EntityTwoId], [t0].[Id0], [t2].[EntityOneId], [t2].[EntityTwoId]");
}

protected override void ClearLog()
Expand Down
Loading

0 comments on commit 5bcdd2e

Please sign in to comment.