Skip to content

Commit

Permalink
Query: Fix ordering for filtered include in single query scenario (#2…
Browse files Browse the repository at this point in the history 
…1337)

Resolves #21338

Also Clean up execution strategy in QueryingEnumerables
Resolves #21211
  • Loading branch information
@smitpatel
smitpatel authored Jun 23, 2020
1 parent 7f3e01a commit c12fd02
Show file tree
Hide file tree
Showing 8 changed files with 71 additions and 49 deletions.
18 changes: 4 additions & 14 deletions src/EFCore.Relational/Query/Internal/FromSqlQueryingEnumerable.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ private sealed class Enumerator : IEnumerator<T>

private RelationalDataReader _dataReader;
private int[] _indexMap;
private IExecutionStrategy _executionStrategy;

public Enumerator(FromSqlQueryingEnumerable<T> queryingEnumerable)
{
Expand All @@ -169,12 +168,8 @@ public bool MoveNext()
{
if (_dataReader == null)
{
if (_executionStrategy == null)
{
_executionStrategy = _relationalQueryContext.ExecutionStrategyFactory.Create();
}

_executionStrategy.Execute(true, InitializeReader, null);
_relationalQueryContext.ExecutionStrategyFactory.Create()
.Execute(true, InitializeReader, null);
}

var hasNext = _dataReader.Read();
Expand Down Expand Up @@ -236,7 +231,6 @@ private sealed class AsyncEnumerator : IAsyncEnumerator<T>

private RelationalDataReader _dataReader;
private int[] _indexMap;
private IExecutionStrategy _executionStrategy;

public AsyncEnumerator(
FromSqlQueryingEnumerable<T> queryingEnumerable,
Expand All @@ -262,12 +256,8 @@ public async ValueTask<bool> MoveNextAsync()
{
if (_dataReader == null)
{
if (_executionStrategy == null)
{
_executionStrategy = _relationalQueryContext.ExecutionStrategyFactory.Create();
}

await _executionStrategy.ExecuteAsync(true, InitializeReaderAsync, null, _cancellationToken).ConfigureAwait(false);
await _relationalQueryContext.ExecutionStrategyFactory.Create()
.ExecuteAsync(true, InitializeReaderAsync, null, _cancellationToken).ConfigureAwait(false);
}

var hasNext = await _dataReader.ReadAsync(_cancellationToken).ConfigureAwait(false);
Expand Down
18 changes: 4 additions & 14 deletions src/EFCore.Relational/Query/Internal/SingleQueryingEnumerable.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ private sealed class Enumerator : IEnumerator<T>

private RelationalDataReader _dataReader;
private SingleQueryResultCoordinator _resultCoordinator;
private IExecutionStrategy _executionStrategy;

public Enumerator(SingleQueryingEnumerable<T> queryingEnumerable)
{
Expand All @@ -137,12 +136,8 @@ public bool MoveNext()
{
if (_dataReader == null)
{
if (_executionStrategy == null)
{
_executionStrategy = _relationalQueryContext.ExecutionStrategyFactory.Create();
}

_executionStrategy.Execute(true, InitializeReader, null);
_relationalQueryContext.ExecutionStrategyFactory.Create()
.Execute(true, InitializeReader, null);
}

var hasNext = _resultCoordinator.HasNext ?? _dataReader.Read();
Expand Down Expand Up @@ -228,7 +223,6 @@ private sealed class AsyncEnumerator : IAsyncEnumerator<T>

private RelationalDataReader _dataReader;
private SingleQueryResultCoordinator _resultCoordinator;
private IExecutionStrategy _executionStrategy;

public AsyncEnumerator(
SingleQueryingEnumerable<T> queryingEnumerable,
Expand All @@ -253,12 +247,8 @@ public async ValueTask<bool> MoveNextAsync()
{
if (_dataReader == null)
{
if (_executionStrategy == null)
{
_executionStrategy = _relationalQueryContext.ExecutionStrategyFactory.Create();
}

await _executionStrategy.ExecuteAsync(true, InitializeReaderAsync, null, _cancellationToken)
await _relationalQueryContext.ExecutionStrategyFactory.Create()
.ExecuteAsync(true, InitializeReaderAsync, null, _cancellationToken)
.ConfigureAwait(false);
}

Expand Down
44 changes: 43 additions & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1227,8 +1227,50 @@ public Expression ApplyCollectionJoin(
}

AddJoin(JoinType.OuterApply, ref innerSelectExpression);
var innerOrderingExpressions = new List<OrderingExpression>();
var joinedTable = innerSelectExpression.Tables.Single();
if (joinedTable is SelectExpression collectionSelectExpression
&& collectionSelectExpression.Predicate != null
&& collectionSelectExpression.Tables.Count == 1
&& collectionSelectExpression.Tables[0] is SelectExpression rowNumberSubquery
&& rowNumberSubquery.Projection.Select(pe => pe.Expression)
.OfType<RowNumberExpression>().SingleOrDefault() is RowNumberExpression rowNumberExpression)
{
foreach (var partition in rowNumberExpression.Partitions)
{
innerOrderingExpressions.Add(new OrderingExpression(
collectionSelectExpression.GenerateOuterColumn(rowNumberSubquery.GenerateOuterColumn(partition)),
ascending: true));
}

foreach (var ordering in rowNumberExpression.Orderings)
{
innerOrderingExpressions.Add(new OrderingExpression(
collectionSelectExpression.GenerateOuterColumn(rowNumberSubquery.GenerateOuterColumn(ordering.Expression)),
ordering.IsAscending));
}
}
else if (joinedTable is SelectExpression collectionSelectExpression2
&& collectionSelectExpression2.Orderings.Count > 0)
{
foreach (var ordering in collectionSelectExpression2.Orderings)
{
if (innerSelectExpression._identifier.Any(e => e.Column.Equals(ordering.Expression)))
{
continue;
}

innerOrderingExpressions.Add(new OrderingExpression(
collectionSelectExpression2.GenerateOuterColumn(ordering.Expression),
ordering.IsAscending));
}
}
else
{
innerOrderingExpressions.AddRange(innerSelectExpression.Orderings);
}

foreach (var ordering in innerSelectExpression.Orderings)
foreach (var ordering in innerOrderingExpressions)
{
AppendOrdering(ordering.Update(MakeNullable(ordering.Expression)));
}
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5140,7 +5140,7 @@ public virtual Task Filtered_include_after_reference_navigation(bool async)
assertOrder: true)));
}

[ConditionalTheory(Skip = "issue #21338")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filtered_include_after_different_filtered_include_same_level(bool async)
{
Expand Down Expand Up @@ -5205,7 +5205,7 @@ public virtual async Task Filtered_include_different_filter_set_on_same_navigati
.Include(l1 => l1.OneToMany_Optional1.Where(x => x.Name != "Bar")).ThenInclude(l2 => l2.OneToOne_Required_FK2)))).Message;
}

[ConditionalTheory(Skip = "issue #21338")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filtered_include_same_filter_set_on_same_navigation_twice(bool async)
{
Expand Down
30 changes: 15 additions & 15 deletions test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4567,7 +4567,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Name], [t0].[Id]");
}

public override async Task Filtered_include_basic_OrderBy_Skip(bool async)
Expand All @@ -4585,7 +4585,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE 1 < [t].[row]
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Name], [t0].[Id]");
}

public override async Task Filtered_include_basic_OrderBy_Skip_Take(bool async)
Expand All @@ -4603,7 +4603,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE (1 < [t].[row]) AND ([t].[row] <= 4)
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Name], [t0].[Id]");
}

public override void Filtered_include_Skip_without_OrderBy()
Expand All @@ -4621,7 +4621,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE 1 < [t].[row]
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override void Filtered_include_Take_without_OrderBy()
Expand All @@ -4639,7 +4639,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override async Task Filtered_include_on_ThenInclude(bool async)
Expand All @@ -4659,7 +4659,7 @@ FROM [LevelThree] AS [l1]
) AS [t]
WHERE (1 < [t].[row]) AND ([t].[row] <= 4)
) AS [t0] ON [l0].[Id] = [t0].[OneToMany_Optional_Inverse3Id]
ORDER BY [l].[Id], [l0].[Id], [t0].[Id]");
ORDER BY [l].[Id], [l0].[Id], [t0].[OneToMany_Optional_Inverse3Id], [t0].[Name], [t0].[Id]");
}

public override async Task Filtered_include_after_reference_navigation(bool async)
Expand All @@ -4679,7 +4679,7 @@ FROM [LevelThree] AS [l1]
) AS [t]
WHERE (1 < [t].[row]) AND ([t].[row] <= 4)
) AS [t0] ON [l0].[Id] = [t0].[OneToMany_Optional_Inverse3Id]
ORDER BY [l].[Id], [l0].[Id], [t0].[Id]");
ORDER BY [l].[Id], [l0].[Id], [t0].[OneToMany_Optional_Inverse3Id], [t0].[Name], [t0].[Id]");
}

public override async Task Filtered_include_after_different_filtered_include_same_level(bool async)
Expand Down Expand Up @@ -4707,7 +4707,7 @@ FROM [LevelTwo] AS [l1]
) AS [t1]
WHERE 1 < [t1].[row]
) AS [t2] ON [l].[Id] = [t2].[OneToMany_Required_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id], [t2].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Name], [t0].[Id], [t2].[OneToMany_Required_Inverse2Id], [t2].[Name] DESC, [t2].[Id]");
}

public override async Task Filtered_include_after_different_filtered_include_different_level(bool async)
Expand Down Expand Up @@ -4735,7 +4735,7 @@ FROM [LevelThree] AS [l1]
WHERE 1 < [t0].[row]
) AS [t1] ON [t].[Id] = [t1].[OneToMany_Required_Inverse3Id]
) AS [t2]
ORDER BY [l].[Id], [t2].[Name], [t2].[Id], [t2].[Id0]");
ORDER BY [l].[Id], [t2].[Name], [t2].[Id], [t2].[OneToMany_Required_Inverse3Id], [t2].[Name0] DESC, [t2].[Id0]");
}

public override async Task Filtered_include_same_filter_set_on_same_navigation_twice(bool async)
Expand All @@ -4754,7 +4754,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 2
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id] DESC");
}

public override async Task Filtered_include_same_filter_set_on_same_navigation_twice_followed_by_ThenIncludes(bool async)
Expand Down Expand Up @@ -4815,7 +4815,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override async Task Filtered_include_and_non_filtered_include_on_same_navigation2(bool async)
Expand All @@ -4834,7 +4834,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override async Task Filtered_include_and_non_filtered_include_followed_by_then_include_on_same_navigation(bool async)
Expand Down Expand Up @@ -4930,7 +4930,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override void Filtered_include_context_accessed_inside_filter()
Expand All @@ -4954,7 +4954,7 @@ FROM [LevelTwo] AS [l0]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override void Filtered_include_context_accessed_inside_filter_correlated()
Expand All @@ -4976,7 +4976,7 @@ FROM [LevelOne] AS [l1]
) AS [t]
WHERE [t].[row] <= 3
) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id]
ORDER BY [l].[Id], [t0].[Id]");
ORDER BY [l].[Id], [t0].[OneToMany_Optional_Inverse2Id], [t0].[Id]");
}

public override void Filtered_include_outer_parameter_used_inside_filter()
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5896,7 +5896,7 @@ FROM [Gears] AS [g0]
WHERE [t0].[row] <= 50
) AS [t1] ON (([g].[Nickname] = [t1].[LeaderNickname]) OR ([g].[Nickname] IS NULL AND [t1].[LeaderNickname] IS NULL)) AND ([g].[SquadId] = [t1].[LeaderSquadId])
WHERE [g].[Discriminator] = N'Officer'
ORDER BY [t].[Id], [g].[Nickname], [g].[SquadId], [t1].[Nickname], [t1].[SquadId]");
ORDER BY [t].[Id], [g].[Nickname], [g].[SquadId], [t1].[LeaderNickname], [t1].[LeaderSquadId], [t1].[Nickname], [t1].[SquadId]");
}

public override async Task Project_collection_navigation_nested_composite_key(bool async)
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4742,7 +4742,7 @@ WHERE DATEPART(year, [o].[OrderDate]) = 1998
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [c].[CustomerID] = [t0].[CustomerID]
ORDER BY [c].[CustomerID], [t0].[OrderID]");
ORDER BY [c].[CustomerID], [t0].[CustomerID], [t0].[OrderID]");
}

public override async Task Subquery_DefaultIfEmpty_Any(bool async)
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ WHERE [o].[OrderID] < 10500
WHERE [t].[row] <= 3
) AS [t0] ON [c].[CustomerID] = [t0].[CustomerID]
WHERE [c].[CustomerID] LIKE N'A%'
ORDER BY [c].[CustomerID], [t0].[OrderID]");
ORDER BY [c].[CustomerID], [t0].[CustomerID], [t0].[OrderID]");
}

public override void Select_nested_collection_multi_level2()
Expand Down

0 comments on commit c12fd02

Please sign in to comment.