Skip to content

Commit

Permalink
Query: Avoid pushdown when joining with a subquery which can be lifte…
Browse files Browse the repository at this point in the history 
…d out

Part of #17622

Avoids pushdown for scenario when we cause pushdown due to order by but then we erase order by without skip take giving us simple and lift-able query again
  • Loading branch information
@smitpatel
smitpatel committed Oct 28, 2021
1 parent f51ea1c commit 0086b54
Show file tree
Hide file tree
Showing 15 changed files with 334 additions and 486 deletions.
119 changes: 84 additions & 35 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ static void UpdateLimit(SelectExpression selectExpression)
innerShaperExpression);
}

AddJoin(JoinType.OuterApply, ref innerSelectExpression);
AddJoin(JoinType.OuterApply, ref innerSelectExpression, out _);
var offset = _clientProjections.Count;
var count = innerSelectExpression._clientProjections.Count;
_clientProjections.AddRange(
Expand Down Expand Up @@ -650,7 +650,7 @@ static Expression RemoveConvert(Expression expression)
#endif
var parentIdentifier = GetIdentifierAccessor(this, newClientProjections, actualParentIdentifier).Item1;

outerSelectExpression.AddCrossApply(innerSelectExpression);
outerSelectExpression.AddJoin(JoinType.CrossApply, ref innerSelectExpression, out var pushdownOccurredWhenJoining);
outerSelectExpression._clientProjections.AddRange(innerSelectExpression._clientProjections);
outerSelectExpression._aliasForClientProjections.AddRange(innerSelectExpression._aliasForClientProjections);
innerSelectExpression = outerSelectExpression;
Expand All @@ -670,18 +670,29 @@ static Expression RemoveConvert(Expression expression)
var innerOrderingExpressions = new List<OrderingExpression>();
if (orderingsToBeErased != null)
{
var subquery = (SelectExpression)collectionJoinedInnerTable;
// Ordering was present but erased so we add again
foreach (var ordering in orderingsToBeErased)
if (pushdownOccurredWhenJoining)
{
innerOrderingExpressions.Add(
new OrderingExpression(
subquery.GenerateOuterColumn(collectionJoinedTableReference, ordering.Expression),
ordering.IsAscending));
// We lift from inner subquery if pushdown occurred with ordering erased
var subquery = (SelectExpression)collectionJoinedInnerTable;
foreach (var ordering in orderingsToBeErased)
{
innerOrderingExpressions.Add(
new OrderingExpression(
subquery.GenerateOuterColumn(collectionJoinedTableReference, ordering.Expression),
ordering.IsAscending));
}
}
else
{
// We copy from inner if pushdown did not happen but ordering was left behind when
// generating join
innerOrderingExpressions.AddRange(orderingsToBeErased);
}
}
else
{
// If orderings were not erased then they must be present in inner
GetOrderingsFromInnerTable(
collectionJoinedInnerTable,
collectionJoinedTableReference,
Expand Down Expand Up @@ -722,20 +733,58 @@ static Expression RemoveConvert(Expression expression)

innerShaperExpression = innerSelectExpression.ApplyProjection(
innerShaperExpression, shapedQueryExpression.ResultCardinality, querySplittingBehavior);
AddJoin(JoinType.OuterApply, ref innerSelectExpression);
var innerOrderingExpressions = new List<OrderingExpression>();

if (!GetOrderingsFromInnerTable(
innerSelectExpression._tables[0],
innerSelectExpression._tableReferences[0],
innerOrderingExpressions))
var containsOrdering = innerSelectExpression.Orderings.Count > 0;
List<OrderingExpression>? orderingsToBeErased = null;
if (containsOrdering
&& innerSelectExpression.Limit == null
&& innerSelectExpression.Offset == null)
{
innerOrderingExpressions.AddRange(innerSelectExpression.Orderings);
orderingsToBeErased = innerSelectExpression.Orderings.ToList();
}
AddJoin(JoinType.OuterApply, ref innerSelectExpression, out var pushdownOccurredWhenJoining);

foreach (var ordering in innerOrderingExpressions)
// Copy over any nested ordering if there were any
if (containsOrdering)
{
AppendOrdering(ordering.Update(MakeNullable(ordering.Expression, nullable: true)));
var collectionJoinedInnerTable = innerSelectExpression._tables[0];
var collectionJoinedTableReference = innerSelectExpression._tableReferences[0];
var innerOrderingExpressions = new List<OrderingExpression>();
if (orderingsToBeErased != null)
{
// Ordering was present but erased so we add again
if (pushdownOccurredWhenJoining)
{
// We lift from inner subquery if pushdown occurred with ordering erased
var subquery = (SelectExpression)collectionJoinedInnerTable;
foreach (var ordering in orderingsToBeErased)
{
innerOrderingExpressions.Add(
new OrderingExpression(
subquery.GenerateOuterColumn(collectionJoinedTableReference, ordering.Expression),
ordering.IsAscending));
}
}
else
{
// We copy from inner if pushdown did not happen but ordering was left behind when
// generating join
innerOrderingExpressions.AddRange(orderingsToBeErased);
}
}
else
{
// If orderings were not erased then they must be present in inner
GetOrderingsFromInnerTable(
collectionJoinedInnerTable,
collectionJoinedTableReference,
innerOrderingExpressions);
}

foreach (var ordering in innerOrderingExpressions)
{
AppendOrdering(ordering.Update(MakeNullable(ordering.Expression, nullable: true)));
}
}

innerShaperExpression = CopyProjectionToOuter(innerSelectExpression, innerShaperExpression);
Expand Down Expand Up @@ -830,11 +879,13 @@ static Expression RemoveConvert(Expression expression)

return shaperExpression;

bool GetOrderingsFromInnerTable(
void GetOrderingsFromInnerTable(
TableExpressionBase tableExpressionBase,
TableReferenceExpression tableReferenceExpression,
List<OrderingExpression> orderings)
{
// If operation was converted to predicate join (inner/left join),
// then ordering will be in rownumber expression
if (tableExpressionBase is SelectExpression joinedSubquery
&& joinedSubquery.Predicate != null
&& joinedSubquery.Tables.Count == 1
Expand Down Expand Up @@ -862,11 +913,9 @@ bool GetOrderingsFromInnerTable(
rowNumberSubquery.GenerateOuterColumn(rowNumberSubqueryTableReference, ordering.Expression)),
ordering.IsAscending));
}

return true;
}

if (tableExpressionBase is SelectExpression collectionSelectExpression
// If operation remained apply then ordering will be in the subquery
else if (tableExpressionBase is SelectExpression collectionSelectExpression
&& collectionSelectExpression.Orderings.Count > 0)
{
foreach (var ordering in collectionSelectExpression.Orderings)
Expand All @@ -876,11 +925,7 @@ bool GetOrderingsFromInnerTable(
collectionSelectExpression.GenerateOuterColumn(tableReferenceExpression, ordering.Expression),
ordering.IsAscending));
}

return true;
}

return false;
}

Expression CopyProjectionToOuter(SelectExpression innerSelectExpression, Expression innerShaperExpression)
Expand Down Expand Up @@ -1917,7 +1962,7 @@ private Expression AddJoin(
Expression innerShaper,
SqlExpression? joinPredicate = null)
{
AddJoin(joinType, ref innerSelectExpression, joinPredicate);
AddJoin(joinType, ref innerSelectExpression, out _, joinPredicate);

var transparentIdentifierType = TransparentIdentifierFactory.Create(outerShaper.Type, innerShaper.Type);
var outerMemberInfo = transparentIdentifierType.GetTypeInfo().GetRequiredDeclaredField("Outer");
Expand Down Expand Up @@ -2023,8 +2068,10 @@ private Expression AddJoin(
private void AddJoin(
JoinType joinType,
ref SelectExpression innerSelectExpression,
out bool innerPushdownOccurred,
SqlExpression? joinPredicate = null)
{
innerPushdownOccurred = false;
// Try to convert Apply to normal join
if (joinType == JoinType.CrossApply
|| joinType == JoinType.OuterApply)
Expand Down Expand Up @@ -2099,7 +2146,9 @@ private void AddJoin(

AddJoin(
joinType == JoinType.CrossApply ? JoinType.InnerJoin : JoinType.LeftJoin,
ref innerSelectExpression, joinPredicate);
ref innerSelectExpression,
out innerPushdownOccurred,
joinPredicate);

return;
}
Expand Down Expand Up @@ -2131,15 +2180,15 @@ private void AddJoin(
joinPredicate = sqlRemappingVisitor.Remap(joinPredicate);
}

if (innerSelectExpression.Orderings.Count > 0
|| innerSelectExpression.Limit != null
if (innerSelectExpression.Limit != null
|| innerSelectExpression.Offset != null
|| innerSelectExpression.IsDistinct
|| innerSelectExpression.Predicate != null
|| innerSelectExpression.Tables.Count > 1
|| innerSelectExpression.GroupBy.Count > 0)
{
joinPredicate = innerSelectExpression.PushdownIntoSubqueryInternal().Remap(joinPredicate);
innerPushdownOccurred = true;
}

if (_identifier.Count > 0
Expand Down Expand Up @@ -2481,7 +2530,7 @@ public void AddInnerJoin(SelectExpression innerSelectExpression, SqlExpression j
Check.NotNull(innerSelectExpression, nameof(innerSelectExpression));
Check.NotNull(joinPredicate, nameof(joinPredicate));

AddJoin(JoinType.InnerJoin, ref innerSelectExpression, joinPredicate);
AddJoin(JoinType.InnerJoin, ref innerSelectExpression, out _, joinPredicate);
}

/// <summary>
Expand All @@ -2494,7 +2543,7 @@ public void AddLeftJoin(SelectExpression innerSelectExpression, SqlExpression jo
Check.NotNull(innerSelectExpression, nameof(innerSelectExpression));
Check.NotNull(joinPredicate, nameof(joinPredicate));

AddJoin(JoinType.LeftJoin, ref innerSelectExpression, joinPredicate);
AddJoin(JoinType.LeftJoin, ref innerSelectExpression, out _, joinPredicate);
}

/// <summary>
Expand All @@ -2505,7 +2554,7 @@ public void AddCrossJoin(SelectExpression innerSelectExpression)
{
Check.NotNull(innerSelectExpression, nameof(innerSelectExpression));

AddJoin(JoinType.CrossJoin, ref innerSelectExpression);
AddJoin(JoinType.CrossJoin, ref innerSelectExpression, out _);
}

/// <summary>
Expand All @@ -2516,7 +2565,7 @@ public void AddCrossApply(SelectExpression innerSelectExpression)
{
Check.NotNull(innerSelectExpression, nameof(innerSelectExpression));

AddJoin(JoinType.CrossApply, ref innerSelectExpression);
AddJoin(JoinType.CrossApply, ref innerSelectExpression, out _);
}

/// <summary>
Expand All @@ -2527,7 +2576,7 @@ public void AddOuterApply(SelectExpression innerSelectExpression)
{
Check.NotNull(innerSelectExpression, nameof(innerSelectExpression));

AddJoin(JoinType.OuterApply, ref innerSelectExpression);
AddJoin(JoinType.OuterApply, ref innerSelectExpression, out _);
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.Relational.Specification.Tests/TestUtilities/TestSqlLoggerFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ public void AssertBaseline(string[] expected, bool assertOrder = true)
const string indent = FileNewLine + " ";

var newBaseLine = $@" AssertSql(
{string.Join("," + indent + "//" + indent, SqlStatements.Take(9).Select(sql => "@\"" + sql.Replace("\"", "\"\"") + "\""))});
{string.Join("," + indent + "//" + indent, SqlStatements.Take(20).Select(sql => "@\"" + sql.Replace("\"", "\"\"") + "\""))});
";

if (SqlStatements.Count > 9)
if (SqlStatements.Count > 20)
{
newBaseLine += "Output truncated.";
}
Expand Down
Loading

0 comments on commit 0086b54

Please sign in to comment.