Skip to content

Commit

Permalink
Query: Map projection properly when joining 2 tables
Browse files Browse the repository at this point in the history
When joining 2 tables in relational, we lift the table from inner select expression and copy over projections. If one of this projection is same as any outer projection then we reuse the same projection. That means the associated indexes in inner shaper will go to different indexes rather than in linear fashion.
Fix is to remember which index each projection went and use that rather than applying an offset.

Resolves #19616
  • Loading branch information
smitpatel committed Mar 22, 2020
1 parent 6f4f078 commit c9ca4f4
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 6 deletions.
29 changes: 23 additions & 6 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -914,9 +914,11 @@ public Expression ApplyCollectionJoin(
}

var indexOffset = _projection.Count;
foreach (var projection in innerSelectExpression.Projection)
var innerProjectionCount = innerSelectExpression.Projection.Count;
var indexMap = new int[innerProjectionCount];
for (var i = 0; i < innerProjectionCount; i++)
{
AddToProjection(MakeNullable(projection.Expression));
indexMap[i] = AddToProjection(MakeNullable(innerSelectExpression.Projection[i].Expression));
}

foreach (var identifier in innerSelectExpression._identifier.Concat(innerSelectExpression._childIdentifiers))
Expand All @@ -926,7 +928,10 @@ public Expression ApplyCollectionJoin(
AppendOrdering(new OrderingExpression(updatedColumn, ascending: true));
}

var shaperRemapper = new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset);
var shaperRemapper = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue19616", out var isEnabled)
&& isEnabled
? new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset)
: new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexMap);
innerShaper = shaperRemapper.Visit(innerShaper);
selfIdentifier = shaperRemapper.Visit(selfIdentifier);

Expand Down Expand Up @@ -961,21 +966,32 @@ private class ShaperRemappingExpressionVisitor : ExpressionVisitor
private readonly SelectExpression _queryExpression;
private readonly SelectExpression _innerSelectExpression;
private readonly int _offset;
private readonly int[] _indexMap;

public ShaperRemappingExpressionVisitor(SelectExpression queryExpression, SelectExpression innerSelectExpression, int offset)
public ShaperRemappingExpressionVisitor(
SelectExpression queryExpression, SelectExpression innerSelectExpression, int offset)
{
_queryExpression = queryExpression;
_innerSelectExpression = innerSelectExpression;
_offset = offset;
}

public ShaperRemappingExpressionVisitor(
SelectExpression queryExpression, SelectExpression innerSelectExpression, int[] indexMap)
{
_queryExpression = queryExpression;
_innerSelectExpression = innerSelectExpression;
_indexMap = indexMap;
}

protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is ProjectionBindingExpression projectionBindingExpression)
{
var oldIndex = (int)GetProjectionIndex(projectionBindingExpression);
var newIndex = _indexMap?[oldIndex] ?? oldIndex + _offset;

return new ProjectionBindingExpression(_queryExpression, oldIndex + _offset, projectionBindingExpression.Type);
return new ProjectionBindingExpression(_queryExpression, newIndex, projectionBindingExpression.Type);
}

if (extensionExpression is EntityShaperExpression entityShaper)
Expand All @@ -985,7 +1001,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
var indexMap = new Dictionary<IProperty, int>();
foreach (var keyValuePair in oldIndexMap)
{
indexMap[keyValuePair.Key] = keyValuePair.Value + _offset;
var oldIndex = keyValuePair.Value;
indexMap[keyValuePair.Key] = _indexMap?[oldIndex] ?? oldIndex + _offset;
}

return new EntityShaperExpression(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -921,5 +921,11 @@ public override Task LastOrDefault_member_access_in_projection_translates_to_ser
{
return base.LastOrDefault_member_access_in_projection_translates_to_server(isAsync);
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task Projecting_multiple_collection_with_same_constant_works(bool async)
{
return base.Projecting_multiple_collection_with_same_constant_works(async);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1492,5 +1492,25 @@ public CustomerWrapper2(Customer customer)
public string City { get; set; }
public Customer Customer { get; }
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Projecting_multiple_collection_with_same_constant_works(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI")
.Select(c => new
{
O1 = c.Orders.Select(e => new { Value = 1 }),
O2 = c.Orders.Select(e => new { AnotherValue = 1 })
}),
assertOrder: true, //single element
elementAsserter: (e, a) =>
{
AssertCollection(e.O1, a.O1, ordered: true);
AssertCollection(e.O2, a.O2, ordered: true);
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1227,5 +1227,18 @@ FROM [Orders] AS [o]
FROM [Customers] AS [c]
WHERE [c].[CustomerID] LIKE N'A%'");
}

public override async Task Projecting_multiple_collection_with_same_constant_works(bool async)
{
await base.Projecting_multiple_collection_with_same_constant_works(async);

AssertSql(
@"SELECT [c].[CustomerID], 1, [o].[OrderID], [o0].[OrderID]
FROM [Customers] AS [c]
LEFT JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
LEFT JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
WHERE [c].[CustomerID] = N'ALFKI'
ORDER BY [c].[CustomerID], [o].[OrderID], [o0].[OrderID]");
}
}
}

0 comments on commit c9ca4f4

Please sign in to comment.