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

Jump to bottom

Query: Lift order by collection queries #16532

Merged
merged 1 commit into from
Jul 9, 2019
Merged

Query: Lift order by collection queries #16532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,21 @@ internal ColumnExpression(IProperty property, TableExpressionBase table, bool nu
{
}

internal ColumnExpression(ProjectionExpression subqueryProjection, TableExpressionBase table, bool nullable)
: this(subqueryProjection.Alias, table, subqueryProjection.Type, subqueryProjection.Expression.TypeMapping, nullable)
internal ColumnExpression(ProjectionExpression subqueryProjection, TableExpressionBase table)
: this(subqueryProjection.Alias, table,
subqueryProjection.Type, subqueryProjection.Expression.TypeMapping,
IsNullableProjection(subqueryProjection))
{
}

private static bool IsNullableProjection(ProjectionExpression projectionExpression)
=> projectionExpression.Expression switch
{
ColumnExpression columnExpression => columnExpression.Nullable,
SqlConstantExpression sqlConstantExpression => sqlConstantExpression.Value == null,
_ => true,
};

private ColumnExpression(string name, TableExpressionBase table, Type type, RelationalTypeMapping typeMapping, bool nullable)
: base(type, typeMapping)
{
Expand Down
83 changes: 40 additions & 43 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ public Expression ApplyGrouping(Expression keySelector)
var subquery = (SelectExpression)_tables[0];
var projectionIndex = subquery.AddToProjection((SqlExpression)keySelector, nameof(IGrouping<int, int>.Key));

keySelector = new ColumnExpression(subquery.Projection[projectionIndex], subquery, false);
keySelector = new ColumnExpression(subquery.Projection[projectionIndex], subquery);
}

AppendGroupBy(keySelector);
Expand Down Expand Up @@ -391,6 +391,7 @@ public void ApplyDistinct()
}

IsDistinct = true;

ClearOrdering();
}

Expand Down Expand Up @@ -456,14 +457,14 @@ public Expression ApplySetOperation(
if (joinedMapping.Value1 is EntityProjectionExpression entityProjection1
&& joinedMapping.Value2 is EntityProjectionExpression entityProjection2)
{
HandleEntityMapping(joinedMapping.Key, select1, entityProjection1, select2, entityProjection2);
handleEntityMapping(joinedMapping.Key, select1, entityProjection1, select2, entityProjection2);
continue;
}

if (joinedMapping.Value1 is ColumnExpression && joinedMapping.Value2 is ColumnExpression
|| joinedMapping.Value1 is SubSelectExpression && joinedMapping.Value2 is SubSelectExpression)
{
HandleColumnMapping(
handleColumnMapping(
joinedMapping.Key,
select1, (SqlExpression)joinedMapping.Value1,
select2, (SqlExpression)joinedMapping.Value2);
Expand All @@ -487,7 +488,7 @@ public Expression ApplySetOperation(

return shaperExpression;

void HandleEntityMapping(
void handleEntityMapping(
Copy link
Member

Choose a reason for hiding this comment

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

@smitpatel @ajcvickers do we have a convention where local functions are camel-cased? From what I can see in samples, pascal case is used just like any other function.

Copy link
Member

Choose a reason for hiding this comment

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

I think they should be pascal case, but we agreed to decide after 3.0.

ProjectionMember projectionMember,
SelectExpression select1, EntityProjectionExpression projection1,
SelectExpression select2, EntityProjectionExpression projection2)
Expand All @@ -498,7 +499,7 @@ void HandleEntityMapping(
{
foreach (var property in GetAllPropertiesInHierarchy(projection1.EntityType))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
propertyExpressions[property] = addSetOperationColumnProjections(
property,
select1, projection1.BindProperty(property),
select2, projection2.BindProperty(property));
Expand All @@ -511,7 +512,7 @@ void HandleEntityMapping(
throw new InvalidOperationException("Set operations over different entity types are currently unsupported (see #16298)");
}

ColumnExpression AddSetOperationColumnProjections(
ColumnExpression addSetOperationColumnProjections(
IProperty property,
SelectExpression select1, ColumnExpression column1,
SelectExpression select2, ColumnExpression column2)
Expand All @@ -529,7 +530,7 @@ ColumnExpression AddSetOperationColumnProjections(
return column1;
}

void HandleColumnMapping(
void handleColumnMapping(
ProjectionMember projectionMember,
SelectExpression select1, SqlExpression innerColumn1,
SelectExpression select2, SqlExpression innerColumn2)
Expand All @@ -545,7 +546,8 @@ void HandleColumnMapping(

public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
{
var subquery = new SelectExpression("t", new List<ProjectionExpression>(), _tables.ToList(), _groupBy.ToList(), _orderings.ToList())
var subquery = new SelectExpression(
"t", new List<ProjectionExpression>(), _tables.ToList(), _groupBy.ToList(), _orderings.ToList())
{
IsDistinct = IsDistinct,
Predicate = Predicate,
Expand All @@ -555,9 +557,11 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
SetOperationType = SetOperationType
};

if (subquery.Limit == null && subquery.Offset == null)
ColumnExpression liftProjectionFromSubquery(SqlExpression projection)
{
subquery.ClearOrdering();
var index = subquery.AddToProjection(projection);
var projectionExpression = subquery._projection[index];
return new ColumnExpression(projectionExpression, subquery);
}

var projectionMap = new Dictionary<SqlExpression, ColumnExpression>();
Expand All @@ -567,9 +571,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
_projection.Clear();
foreach (var projection in projections)
{
var index = subquery.AddToProjection(projection);
var projectionExpression = subquery._projection[index];
var outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
var outerColumn = liftProjectionFromSubquery(projection);
AddToProjection(outerColumn);
projectionMap[projection] = outerColumn;
}
Expand All @@ -584,9 +586,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
var innerColumn = entityProjection.BindProperty(property);
var index = subquery.AddToProjection(innerColumn);
var projectionExpression = subquery._projection[index];
var outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
var outerColumn = liftProjectionFromSubquery(innerColumn);
projectionMap[innerColumn] = outerColumn;
propertyExpressions[property] = outerColumn;
}
Expand All @@ -596,9 +596,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
else
{
var innerColumn = (SqlExpression)mapping.Value;
var index = subquery.AddToProjection(innerColumn);
var projectionExpression = subquery._projection[index];
var outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
var outerColumn = liftProjectionFromSubquery(innerColumn);
projectionMap[innerColumn] = outerColumn;
_projectionMapping[mapping.Key] = outerColumn;
}
Expand All @@ -616,9 +614,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
}
else if (!IsDistinct && GroupBy.Count == 0)
{
var index = subquery.AddToProjection(identifier);
var projectionExpression = subquery._projection[index];
outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
outerColumn = liftProjectionFromSubquery(identifier);
_identifier.Add(outerColumn);
}
}
Expand All @@ -634,9 +630,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
}
else if (!IsDistinct && GroupBy.Count == 0)
{
var index = subquery.AddToProjection(identifier);
var projectionExpression = subquery._projection[index];
outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
outerColumn = liftProjectionFromSubquery(identifier);
_childIdentifiers.Add(outerColumn);
}
}
Expand All @@ -646,16 +640,24 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
_pendingCollections.AddRange(pendingCollections.Select(new SqlRemappingVisitor(projectionMap).Remap));

_orderings.Clear();
foreach (var ordering in subquery._orderings)
// Only lift order by to outer if subquery does not have distinct
if (!subquery.IsDistinct)
{
var orderingExpression = ordering.Expression;
if (!projectionMap.TryGetValue(orderingExpression, out var outerColumn))
foreach (var ordering in subquery._orderings)
{
var index = subquery.AddToProjection(orderingExpression);
var projectionExpression = subquery._projection[index];
outerColumn = new ColumnExpression(projectionExpression, subquery, IsNullableProjection(projectionExpression));
var orderingExpression = ordering.Expression;
if (!projectionMap.TryGetValue(orderingExpression, out var outerColumn))
{
outerColumn = liftProjectionFromSubquery(orderingExpression);
}

_orderings.Add(ordering.Update(outerColumn));
}
_orderings.Add(new OrderingExpression(outerColumn, ordering.Ascending));
}

if (subquery.Offset == null && subquery.Limit == null)
{
subquery.ClearOrdering();
}

Offset = null;
Expand All @@ -671,11 +673,6 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
return projectionMap;
}

private static bool IsNullableProjection(ProjectionExpression projection)
{
return projection.Expression is ColumnExpression column ? column.Nullable : true;
}

public CollectionShaperExpression AddCollectionProjection(
ShapedQueryExpression shapedQueryExpression, INavigation navigation, Type elementType)
{
Expand Down Expand Up @@ -717,18 +714,18 @@ public Expression ApplyCollectionJoin(
|| innerSelectExpression.Tables.Count > 1
|| innerSelectExpression.GroupBy.Count > 1)
{
var orderings = innerSelectExpression.Orderings.ToList();
var sqlRemappingVisitor = new SqlRemappingVisitor(innerSelectExpression.PushdownIntoSubquery());
joinPredicate = sqlRemappingVisitor.Remap(joinPredicate);

foreach (var ordering in orderings)
{
AppendOrdering(ordering.Update(MakeNullable(sqlRemappingVisitor.Remap(ordering.Expression))));
}
}

var leftJoinExpression = new LeftJoinExpression(innerSelectExpression.Tables.Single(), joinPredicate);
_tables.Add(leftJoinExpression);

foreach (var ordering in innerSelectExpression.Orderings)
{
AppendOrdering(ordering.Update(MakeNullable(ordering.Expression)));
}

var indexOffset = _projection.Count;
foreach (var projection in innerSelectExpression.Projection)
{
Expand Down
16 changes: 8 additions & 8 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4661,7 +4661,7 @@ where ps.SquadId < 7
});
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_nested_with_custom_ordering(bool isAsync)
{
Expand Down Expand Up @@ -4749,7 +4749,7 @@ orderby g.Rank
});
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory(Skip = "Issue#16318")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_different_collections_projected(bool isAsync)
{
Expand Down Expand Up @@ -4797,7 +4797,7 @@ where o.Reports.Any()
assertOrder: true);
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Multiple_orderby_with_navigation_expansion_on_one_of_the_order_bys_inside_subquery(bool isAsync)
{
Expand Down Expand Up @@ -4825,7 +4825,7 @@ where o.Reports.Any()
});
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Multiple_orderby_with_navigation_expansion_on_one_of_the_order_bys_inside_subquery_duplicated_orderings(bool isAsync)
{
Expand Down Expand Up @@ -4855,7 +4855,7 @@ where o.Reports.Any()
}


[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Multiple_orderby_with_navigation_expansion_on_one_of_the_order_bys_inside_subquery_complex_orderings(bool isAsync)
{
Expand Down Expand Up @@ -4883,7 +4883,7 @@ where o.Reports.Any()
});
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_multiple_nested_complex_collections(bool isAsync)
{
Expand Down Expand Up @@ -5284,7 +5284,7 @@ from g in grouping.DefaultIfEmpty()
eee => eee.Id, (eee, aaa) => Assert.Equal(eee.Id, aaa.Id))));
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_from_left_join_with_additional_elements_projected_of_that_join(bool isAsync)
{
Expand Down Expand Up @@ -5442,7 +5442,7 @@ from r in gs
});
}

[ConditionalTheory(Skip = "Issue#16226")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_with_funky_orderby_complex_scenario2(bool isAsync)
{
Expand Down
Loading