Skip to content

Commit

Permalink
Query: Lift projectionMapping always when pushing down SelectExpressi…
Browse files Browse the repository at this point in the history 
…on into subuquery

Issue: When converting correlation to join, we add the join key columns to projection to make sure they are always available.
We had logic that if projection contains any columns then projection mapping is already applied to projection.
But in above case that is incorrect.
That leaves us with incorrect EntityProjection which fails in further translation as multi-part identifier could not be bound.

We should always lift projectionMapping during Pushdown unless projectionMapping is applied which can be checked through type.

This scenario does not work for owned/weak types due to #18519

Resolves #18514
  • Loading branch information
@smitpatel
smitpatel committed Oct 23, 2019
1 parent d3850b8 commit 22679cf
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 59 deletions.
99 changes: 52 additions & 47 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,19 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions
{
public class SelectExpression : TableExpressionBase
{
private IDictionary<ProjectionMember, Expression> _projectionMapping = new Dictionary<ProjectionMember, Expression>();
private readonly List<ProjectionExpression> _projection = new List<ProjectionExpression>();

private readonly IDictionary<EntityProjectionExpression, IDictionary<IProperty, int>> _entityProjectionCache
= new Dictionary<EntityProjectionExpression, IDictionary<IProperty, int>>();

private readonly List<ProjectionExpression> _projection = new List<ProjectionExpression>();
private readonly List<TableExpressionBase> _tables = new List<TableExpressionBase>();
private readonly List<SqlExpression> _groupBy = new List<SqlExpression>();
private readonly List<OrderingExpression> _orderings = new List<OrderingExpression>();

private readonly List<SqlExpression> _identifier = new List<SqlExpression>();
private readonly List<SqlExpression> _childIdentifiers = new List<SqlExpression>();
private readonly List<SelectExpression> _pendingCollections = new List<SelectExpression>();

private IDictionary<ProjectionMember, Expression> _projectionMapping = new Dictionary<ProjectionMember, Expression>();

public IReadOnlyList<ProjectionExpression> Projection => _projection;
public IReadOnlyList<TableExpressionBase> Tables => _tables;
public IReadOnlyList<SqlExpression> GroupBy => _groupBy;
Expand Down Expand Up @@ -137,7 +136,7 @@ public void ApplyProjection()
_projectionMapping = result;
}

private IEnumerable<IProperty> GetAllPropertiesInHierarchy(IEntityType entityType)
private static IEnumerable<IProperty> GetAllPropertiesInHierarchy(IEntityType entityType)
=> entityType.GetTypesInHierarchy().SelectMany(EntityTypeExtensions.GetDeclaredProperties);

public void ReplaceProjectionMapping(IDictionary<ProjectionMember, Expression> projectionMapping)
Expand Down Expand Up @@ -660,35 +659,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()

var projectionMap = new Dictionary<SqlExpression, ColumnExpression>();

EntityProjectionExpression liftEntityProjectionFromSubquery(EntityProjectionExpression entityProjection)
{
var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
var innerColumn = entityProjection.BindProperty(property);
var outerColumn = subquery.GenerateOuterColumn(innerColumn);
projectionMap[innerColumn] = outerColumn;
propertyExpressions[property] = outerColumn;
}

var newEntityProjection = new EntityProjectionExpression(entityProjection.EntityType, propertyExpressions);
// Also lift nested entity projections
foreach (var navigation in entityProjection.EntityType.GetTypesInHierarchy()
.SelectMany(EntityTypeExtensions.GetDeclaredNavigations))
{
var boundEntityShaperExpression = entityProjection.BindNavigation(navigation);
if (boundEntityShaperExpression != null)
{
var innerEntityProjection = (EntityProjectionExpression)boundEntityShaperExpression.ValueBufferExpression;
var newInnerEntityProjection = liftEntityProjectionFromSubquery(innerEntityProjection);
boundEntityShaperExpression = boundEntityShaperExpression.Update(newInnerEntityProjection);
newEntityProjection.AddNavigationBinding(navigation, boundEntityShaperExpression);
}
}

return newEntityProjection;
}

// Projections may be present if added by lifting SingleResult/Enumerable in projection through join
if (_projection.Any())
{
var projections = _projection.Select(pe => pe.Expression).ToList();
Expand All @@ -700,21 +671,26 @@ EntityProjectionExpression liftEntityProjectionFromSubquery(EntityProjectionExpr
projectionMap[projection] = outerColumn;
}
}
else

foreach (var mapping in _projectionMapping.ToList())
{
foreach (var mapping in _projectionMapping.ToList())
// If projectionMapping's value is ConstantExpression then projection has already been applied
// And captured in _projections above so we don't need to process this.
if (mapping.Value is ConstantExpression)
{
if (mapping.Value is EntityProjectionExpression entityProjection)
{
_projectionMapping[mapping.Key] = liftEntityProjectionFromSubquery(entityProjection);
}
else
{
var innerColumn = (SqlExpression)mapping.Value;
var outerColumn = subquery.GenerateOuterColumn(innerColumn);
projectionMap[innerColumn] = outerColumn;
_projectionMapping[mapping.Key] = outerColumn;
}
break;
}

if (mapping.Value is EntityProjectionExpression entityProjection)
{
_projectionMapping[mapping.Key] = LiftEntityProjectionFromSubquery(entityProjection);
}
else
{
var innerColumn = (SqlExpression)mapping.Value;
var outerColumn = subquery.GenerateOuterColumn(innerColumn);
projectionMap[innerColumn] = outerColumn;
_projectionMapping[mapping.Key] = outerColumn;
}
}

Expand Down Expand Up @@ -788,6 +764,35 @@ EntityProjectionExpression liftEntityProjectionFromSubquery(EntityProjectionExpr
_groupBy.Clear();

return projectionMap;

EntityProjectionExpression LiftEntityProjectionFromSubquery(EntityProjectionExpression entityProjection)
{
var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
var innerColumn = entityProjection.BindProperty(property);
var outerColumn = subquery.GenerateOuterColumn(innerColumn);
projectionMap[innerColumn] = outerColumn;
propertyExpressions[property] = outerColumn;
}

var newEntityProjection = new EntityProjectionExpression(entityProjection.EntityType, propertyExpressions);
// Also lift nested entity projections
foreach (var navigation in entityProjection.EntityType.GetTypesInHierarchy()
.SelectMany(EntityTypeExtensions.GetDeclaredNavigations))
{
var boundEntityShaperExpression = entityProjection.BindNavigation(navigation);
if (boundEntityShaperExpression != null)
{
var innerEntityProjection = (EntityProjectionExpression)boundEntityShaperExpression.ValueBufferExpression;
var newInnerEntityProjection = LiftEntityProjectionFromSubquery(innerEntityProjection);
boundEntityShaperExpression = boundEntityShaperExpression.Update(newInnerEntityProjection);
newEntityProjection.AddNavigationBinding(navigation, boundEntityShaperExpression);
}
}

return newEntityProjection;
}
}

public Expression AddSingleProjection(ShapedQueryExpression shapedQueryExpression)
Expand Down
6 changes: 6 additions & 0 deletions test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,11 @@ public override void Member_pushdown_chain_3_levels_deep_entity()
{
base.Member_pushdown_chain_3_levels_deep_entity();
}

[ConditionalTheory(Skip = "issue #17620")]
public override Task Lift_projection_mapping_when_pushing_down_subquery(bool isAsync)
{
return base.Lift_projection_mapping_when_pushing_down_subquery(isAsync);
}
}
}
48 changes: 36 additions & 12 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,18 @@
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;

// ReSharper disable ConvertClosureToMethodGroup
// ReSharper disable PossibleUnintendedReferenceComparison
// ReSharper disable ArgumentsStyleLiteral
// ReSharper disable PossibleMultipleEnumeration
// ReSharper disable UnusedVariable
// ReSharper disable EqualExpressionComparison
// ReSharper disable AccessToDisposedClosure
// ReSharper disable StringStartsWithIsCultureSpecific
// ReSharper disable InconsistentNaming
// ReSharper disable MergeConditionalExpression
// ReSharper disable ReplaceWithSingleCallToSingle
// ReSharper disable ReturnValueOfPureMethodIsNotUsed
// ReSharper disable ConvertClosureToMethodGroup
// ReSharper disable PossibleUnintendedReferenceComparison
// ReSharper disable ArgumentsStyleLiteral
// ReSharper disable PossibleMultipleEnumeration
// ReSharper disable UnusedVariable
// ReSharper disable EqualExpressionComparison
// ReSharper disable AccessToDisposedClosure
// ReSharper disable StringStartsWithIsCultureSpecific
// ReSharper disable InconsistentNaming
// ReSharper disable MergeConditionalExpression
// ReSharper disable ReplaceWithSingleCallToSingle
// ReSharper disable ReturnValueOfPureMethodIsNotUsed
// ReSharper disable ConvertToExpressionBodyWhenPossible
#pragma warning disable RCS1155 // Use StringComparison when comparing strings.
namespace Microsoft.EntityFrameworkCore.Query
Expand Down Expand Up @@ -5725,5 +5725,29 @@ public virtual void Union_over_entities_with_different_nullability()

var result = query.ToList();
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Lift_projection_mapping_when_pushing_down_subquery(bool isAsync)
{
return AssertQuery(
isAsync,
ss => ss.Set<Level1>()
.Take(25)
.Select(l1 => new
{
l1.Id,
c1 = l1.OneToMany_Required1.Select(l2 => new { l2.Id }).FirstOrDefault(),
c2 = l1.OneToMany_Required1.Select(l2 => new { l2.Id })
}),
elementSorter: t => t.Id,
elementAsserter: (e, a) =>
{
Assert.Equal(e.Id, a.Id);
// See issue#17287
Assert.Equal(e.c1?.Id ?? 0, a.c1?.Id);
AssertCollection(e.c2, a.c2, elementSorter: i => i.Id, elementAsserter: (ie, ia) => Assert.Equal(ie.Id, ia.Id));
});
}
}
}
6 changes: 6 additions & 0 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,5 +178,11 @@ public override Task Include_inside_subquery(bool isAsync)
{
return base.Include_inside_subquery(isAsync);
}

[ConditionalTheory(Skip = "Issue#18519")]
public override Task Lift_projection_mapping_when_pushing_down_subquery(bool isAsync)
{
return base.Lift_projection_mapping_when_pushing_down_subquery(isAsync);
}
}
}
24 changes: 24 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4375,6 +4375,30 @@ WHERE [l2].[Id] IS NULL
) AS [t]");
}

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

AssertSql(
@"@__p_0='25'
SELECT [t].[Id], [t1].[Id], [l1].[Id]
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]
) AS [t]
LEFT JOIN (
SELECT [t0].[Id], [t0].[OneToMany_Required_Inverse2Id]
FROM (
SELECT [l0].[Id], [l0].[OneToMany_Required_Inverse2Id], ROW_NUMBER() OVER(PARTITION BY [l0].[OneToMany_Required_Inverse2Id] ORDER BY [l0].[Id]) AS [row]
FROM [LevelTwo] AS [l0]
) AS [t0]
WHERE [t0].[row] <= 1
) AS [t1] ON [t].[Id] = [t1].[OneToMany_Required_Inverse2Id]
LEFT JOIN [LevelTwo] AS [l1] ON [t].[Id] = [l1].[OneToMany_Required_Inverse2Id]
ORDER BY [t].[Id], [l1].[Id]");
}

private void AssertSql(params string[] expected)
{
Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
Expand Down

0 comments on commit 22679cf

Please sign in to comment.