Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove last ORDER BY for collection joins #24360

Merged
6 commits merged into from
Aug 12, 2021
+789 −766
Merged

Remove last ORDER BY for collection joins #24360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c59244
Remove last ORDER BY for collection joins
roji Mar 9, 2021
5a37a7c
Review comments
roji Aug 11, 2021
e1c780a
Revert "Review comments"
roji Aug 11, 2021
1b9e48f
Refactor pending ordering to be self-contained in ApplyProjection
roji Aug 12, 2021
49a9949
Address review comments
roji Aug 12, 2021
82e595f
Address review comments
roji Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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