From 2e9e4560c4b0bc4adac67e8254b7381949f26556 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Tue, 22 Oct 2019 13:42:14 -0700 Subject: [PATCH] Query: Lift projectionMapping always when pushing down SelectExpression 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 --- .../Query/SqlExpressions/SelectExpression.cs | 99 ++++++++++--------- .../ComplexNavigationsQueryInMemoryTest.cs | 6 ++ .../Query/ComplexNavigationsQueryTestBase.cs | 48 ++++++--- .../ComplexNavigationsWeakQueryTestBase.cs | 6 ++ .../ComplexNavigationsQuerySqlServerTest.cs | 24 +++++ 5 files changed, 124 insertions(+), 59 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index dcd3b52aa47..0e31cee000a 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -13,20 +13,19 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions { public class SelectExpression : TableExpressionBase { - private IDictionary _projectionMapping = new Dictionary(); - private readonly List _projection = new List(); - private readonly IDictionary> _entityProjectionCache = new Dictionary>(); + private readonly List _projection = new List(); private readonly List _tables = new List(); private readonly List _groupBy = new List(); private readonly List _orderings = new List(); - private readonly List _identifier = new List(); private readonly List _childIdentifiers = new List(); private readonly List _pendingCollections = new List(); + private IDictionary _projectionMapping = new Dictionary(); + public IReadOnlyList Projection => _projection; public IReadOnlyList Tables => _tables; public IReadOnlyList GroupBy => _groupBy; @@ -137,7 +136,7 @@ public void ApplyProjection() _projectionMapping = result; } - private IEnumerable GetAllPropertiesInHierarchy(IEntityType entityType) + private static IEnumerable GetAllPropertiesInHierarchy(IEntityType entityType) => entityType.GetTypesInHierarchy().SelectMany(EntityTypeExtensions.GetDeclaredProperties); public void ReplaceProjectionMapping(IDictionary projectionMapping) @@ -660,35 +659,7 @@ public IDictionary PushdownIntoSubquery() var projectionMap = new Dictionary(); - EntityProjectionExpression liftEntityProjectionFromSubquery(EntityProjectionExpression entityProjection) - { - var propertyExpressions = new Dictionary(); - 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(); @@ -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; } } @@ -788,6 +764,35 @@ EntityProjectionExpression liftEntityProjectionFromSubquery(EntityProjectionExpr _groupBy.Clear(); return projectionMap; + + EntityProjectionExpression LiftEntityProjectionFromSubquery(EntityProjectionExpression entityProjection) + { + var propertyExpressions = new Dictionary(); + 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) diff --git a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs index 1a290d4432b..47b3314e76e 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs @@ -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); + } } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs index 5993c508f69..06610af5479 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs @@ -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 @@ -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() + .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)); + }); + } } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs index 4cb29354172..ff076cde1bc 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs @@ -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); + } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs index cfd2179a3f7..632f24495f5 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs @@ -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);