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 1 commit
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
31 changes: 29 additions & 2 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 @@ -49,6 +49,7 @@ public sealed partial class SelectExpression : TableExpressionBase
private readonly List<TableReferenceExpression> _tableReferences = new();
private readonly List<SqlExpression> _groupBy = new();
private readonly List<OrderingExpression> _orderings = new();
private OrderingExpression? _pendingOrdering;
private HashSet<string> _usedAliases = new();

private readonly List<(ColumnExpression Column, ValueComparer Comparer)> _identifier = new();
Expand Down Expand Up @@ -732,7 +733,7 @@ static Expression RemoveConvert(Expression expression)
{
var updatedColumn = identifier.Column.MakeNullable();
_childIdentifiers.Add((updatedColumn, identifier.Comparer));
AppendOrdering(new OrderingExpression(updatedColumn, ascending: true));
AppendOrdering(new OrderingExpression(updatedColumn, ascending: true), isPending: true);
}

var result = new SingleCollectionInfo(
Expand Down Expand Up @@ -1206,13 +1207,39 @@ public void ApplyOrdering(OrderingExpression orderingExpression)
/// </summary>
/// <param name="orderingExpression"> An ordering expression to use for ordering. </param>
public void AppendOrdering(OrderingExpression orderingExpression)
=> AppendOrdering(orderingExpression, isPending: false);

private void AppendOrdering(OrderingExpression orderingExpression, bool isPending)
{
Check.NotNull(orderingExpression, nameof(orderingExpression));

if (orderingExpression.Equals(_pendingOrdering))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to de-dupe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should apply right away. The purpose of pending ordering to remember last ordering we would actually end up putting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should apply right away.

Also was not sure. But yeah, it seems pretty sure that trying to append an ordering with pending=true when the same ordering is already pending should just do nothing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to de-dupe

Looking at this again, I'm not sure this is needed...

  • If X is in _pendingOrdering, it already passed dedup to get in there in the last round.
  • If anything else was appended since (pending or not), X would be appended to the actual orderings at that point.
  • If X comes in exactly when X is pending (nothing in between):
    • If the 2nd X comes in with pending=true, we need to append
    • If the 2nd X also comes in pending, I'm not sure. However, that seems edge-casey, and appending when we don't have to isn't a bug, just a missed optimization...

Am I missing something? Do you have a specific scenario in mind?

{
AppendOrderingCore(_pendingOrdering);
_pendingOrdering = null;
return;
}

if (_orderings.FirstOrDefault(o => o.Expression.Equals(orderingExpression.Expression)) == null)
{
_orderings.Add(orderingExpression.Update(AssignUniqueAliases(orderingExpression.Expression)));
if (_pendingOrdering is not null)
{
AppendOrderingCore(_pendingOrdering);
_pendingOrdering = null;
}

if (isPending)
{
_pendingOrdering = orderingExpression;
}
else
{
AppendOrderingCore(orderingExpression);
}
}

void AppendOrderingCore(OrderingExpression orderingExpression)
=> _orderings.Add(orderingExpression.Update(AssignUniqueAliases(orderingExpression.Expression)));
}

/// <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