Skip to content

Commit

Permalink
Query: Avoid pushdown on top level during split query (#24790)
Browse files Browse the repository at this point in the history 
Resolves #24745
  • Loading branch information
@smitpatel
smitpatel authored Apr 28, 2021
1 parent 6f7f730 commit 1f9834a
Show file tree
Hide file tree
Showing 6 changed files with 236 additions and 330 deletions.
70 changes: 59 additions & 11 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,28 @@ public Expression ApplyProjection(
EntityShaperNullableMarkingExpressionVisitor? entityShaperNullableMarkingExpressionVisitor = null;
CloningExpressionVisitor? cloningExpressionVisitor = null;
var pushdownOccurred = false;
if (_clientProjections.Any(e => e is ShapedQueryExpression))
var containsCollection = false;
var containsSingleResult = false;
foreach (var projection in _clientProjections)
{
if (projection is ShapedQueryExpression sqe)
{
if (sqe.ResultCardinality == ResultCardinality.Enumerable)
{
containsCollection = true;
}
if (sqe.ResultCardinality == ResultCardinality.Single
|| sqe.ResultCardinality == ResultCardinality.SingleOrDefault)
{
containsSingleResult = true;
}
}
}

if (containsSingleResult
|| (querySplittingBehavior == QuerySplittingBehavior.SingleQuery && containsCollection))
{
// Pushdown outer since we will be adding join to this
if (Limit != null
|| Offset != null
|| IsDistinct
Expand All @@ -420,24 +440,41 @@ public Expression ApplyProjection(
PushdownIntoSubqueryInternal();
pushdownOccurred = true;
}

entityShaperNullableMarkingExpressionVisitor = new();
}

if (containsSingleResult || containsCollection)
{
cloningExpressionVisitor = new();
}

SelectExpression? baseSelectExpression = null;
if (querySplittingBehavior == QuerySplittingBehavior.SplitQuery
&& _clientProjections.Any(e => e is ShapedQueryExpression sqe && sqe.ResultCardinality == ResultCardinality.Enumerable))
if (querySplittingBehavior == QuerySplittingBehavior.SplitQuery && containsCollection)
{
baseSelectExpression = (SelectExpression)cloningExpressionVisitor!.Visit(this);
if (pushdownOccurred
&& (resultCardinality == ResultCardinality.Single
|| resultCardinality == ResultCardinality.SingleOrDefault)
&& baseSelectExpression.Tables[0] is SelectExpression nestedSelectExpression
&& nestedSelectExpression.Limit is SqlConstantExpression limitConstantExpression
&& limitConstantExpression.Value is int limitValue
&& limitValue == 2)
if (resultCardinality == ResultCardinality.Single
|| resultCardinality == ResultCardinality.SingleOrDefault)
{
nestedSelectExpression.Limit = new SqlConstantExpression(Constant(1), limitConstantExpression.TypeMapping);
// Update limit since split queries don't need limit 2
if (pushdownOccurred)
{
UpdateLimit((SelectExpression)baseSelectExpression.Tables[0]);
}
else
{
UpdateLimit(baseSelectExpression);
}

static void UpdateLimit(SelectExpression selectExpression)
{
if (selectExpression.Limit is SqlConstantExpression limitConstantExpression
&& limitConstantExpression.Value is int limitValue
&& limitValue == 2)
{
selectExpression.Limit = new SqlConstantExpression(Constant(1), limitConstantExpression.TypeMapping);
}
}
}
}

Expand Down Expand Up @@ -569,6 +606,17 @@ static Expression RemoveConvert(Expression expression)
innerSelectExpression = (SelectExpression)new ColumnExpressionReplacingExpressionVisitor(this, outerSelectExpression)
.Visit(innerSelectExpression);

if (outerSelectExpression.Limit != null
|| outerSelectExpression.Offset != null
|| outerSelectExpression.IsDistinct
|| outerSelectExpression._groupBy.Count > 0)
{
// We do pushdown after making sure that inner contains references to outer only
// so that when we do pushdown, we can update inner and maintain graph
var sqlRemappingVisitor = outerSelectExpression.PushdownIntoSubqueryInternal();
innerSelectExpression = sqlRemappingVisitor.Remap(innerSelectExpression);
}

var actualParentIdentifier = _identifier.Take(outerSelectExpression._identifier.Count).ToList();
var containsOrdering = innerSelectExpression.Orderings.Count > 0;
List<OrderingExpression>? orderingsToBeErased = null;
Expand Down
55 changes: 18 additions & 37 deletions ...e.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1389,13 +1389,9 @@ public override async Task Take_Select_collection_Take(bool async)
AssertSql(
@"@__p_0='1'
SELECT [t].[Id], [t].[Name]
FROM (
SELECT TOP(@__p_0) [l].[Id], [l].[Name]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]
) AS [t]
ORDER BY [t].[Id]",
SELECT TOP(@__p_0) [l].[Id], [l].[Name]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]",
//
@"@__p_0='1'
Expand Down Expand Up @@ -1425,14 +1421,10 @@ public override async Task Skip_Take_Select_collection_Skip_Take(bool async)
AssertSql(
@"@__p_0='1'
SELECT [t].[Id], [t].[Name]
FROM (
SELECT [l].[Id], [l].[Name]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]
OFFSET @__p_0 ROWS FETCH NEXT @__p_0 ROWS ONLY
) AS [t]
ORDER BY [t].[Id]",
SELECT [l].[Id], [l].[Name]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]
OFFSET @__p_0 ROWS FETCH NEXT @__p_0 ROWS ONLY",
//
@"@__p_0='1'
Expand Down Expand Up @@ -2539,13 +2531,9 @@ public override async Task Filtered_include_Take_with_another_Take_on_top_level(
AssertSql(
@"@__p_0='5'
SELECT [t].[Id], [t].[Date], [t].[Name], [t].[OneToMany_Optional_Self_Inverse1Id], [t].[OneToMany_Required_Self_Inverse1Id], [t].[OneToOne_Optional_Self1Id]
FROM (
SELECT TOP(@__p_0) [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]
) AS [t]
ORDER BY [t].[Id]",
SELECT TOP(@__p_0) [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id]",
//
@"@__p_0='5'
Expand Down Expand Up @@ -2576,14 +2564,10 @@ public override async Task Filtered_include_Skip_Take_with_another_Skip_Take_on_
@"@__p_0='10'
@__p_1='5'
SELECT [t].[Id], [t].[Date], [t].[Name], [t].[OneToMany_Optional_Self_Inverse1Id], [t].[OneToMany_Required_Self_Inverse1Id], [t].[OneToOne_Optional_Self1Id]
FROM (
SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id] DESC
OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY
) AS [t]
ORDER BY [t].[Id] DESC",
SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
ORDER BY [l].[Id] DESC
OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY",
//
@"@__p_0='10'
@__p_1='5'
Expand Down Expand Up @@ -2614,13 +2598,10 @@ public override async Task Projecting_collection_with_FirstOrDefault(bool async)
await base.Projecting_collection_with_FirstOrDefault(async);

AssertSql(
@"SELECT [t].[Id]
FROM (
SELECT TOP(1) [l].[Id]
FROM [LevelOne] AS [l]
WHERE [l].[Id] = 1
) AS [t]
ORDER BY [t].[Id]",
@"SELECT TOP(1) [l].[Id]
FROM [LevelOne] AS [l]
WHERE [l].[Id] = 1
ORDER BY [l].[Id]",
//
@"SELECT [l0].[Id], [l0].[Date], [l0].[Level1_Optional_Id], [l0].[Level1_Required_Id], [l0].[Name], [l0].[OneToMany_Optional_Inverse2Id], [l0].[OneToMany_Optional_Self_Inverse2Id], [l0].[OneToMany_Required_Inverse2Id], [l0].[OneToMany_Required_Self_Inverse2Id], [l0].[OneToOne_Optional_PK_Inverse2Id], [l0].[OneToOne_Optional_Self2Id], [t].[Id]
FROM (
Expand Down
10 changes: 3 additions & 7 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindQueryTaggingQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,9 @@ var customer
AssertSql(
@"-- Yanni
SELECT [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region]
FROM (
SELECT TOP(1) [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
ORDER BY [c].[CustomerID]
) AS [t]
ORDER BY [t].[CustomerID]",
SELECT TOP(1) [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
ORDER BY [c].[CustomerID]",
//
@"-- Yanni
Expand Down
Loading

0 comments on commit 1f9834a

Please sign in to comment.