From 311e5ccb863342a476eb38aacd420b5283f2f7f9 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Tue, 17 Dec 2019 17:40:34 -0800 Subject: [PATCH] Query: Lift pending collections property when applying single result projection Resolves #18812 Issue: When generating left join for single result using RowNumber, pending collection from inner was bubbling up but we lost it when generating join. Later when we encountered CollectionShaperExpression, we threw exception since projectionBindingExpression there references collectionId rather than actual projection Fix: Copy over pending collection for join. Remap collection shapers if any. --- .../Query/SqlExpressions/SelectExpression.cs | 66 ++++++++++++------- .../ComplexNavigationsQueryInMemoryTest.cs | 12 ++++ ...ComplexNavigationsWeakQueryInMemoryTest.cs | 12 ++++ .../Query/ComplexNavigationsQueryTestBase.cs | 61 +++++++++++++++++ .../ComplexNavigationsQuerySqlServerTest.cs | 42 ++++++++++++ 5 files changed, 169 insertions(+), 24 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 83891d3f1bc..70497e09b13 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -871,6 +871,7 @@ public Expression AddSingleProjection([NotNull] ShapedQueryExpression shapedQuer innerSelectExpression.ApplyProjection(); var projectionCount = innerSelectExpression.Projection.Count; + var pendingCollectionOffset = _pendingCollections.Count; AddOuterApply(innerSelectExpression, null); // Joined SelectExpression may different based on left join or outer apply @@ -885,7 +886,8 @@ public Expression AddSingleProjection([NotNull] ShapedQueryExpression shapedQuer AddToProjection(MakeNullable(addedSelectExperssion.GenerateOuterColumn(projection.Expression))); } - return new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset) + // We move pendingCollectionOffset if one was lifted from inner. + return new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset, pendingCollectionOffset) .Visit(shaperExpression); static Expression RemoveConvert(Expression expression) @@ -980,7 +982,9 @@ public Expression ApplyCollectionJoin( AppendOrdering(new OrderingExpression(updatedColumn, ascending: true)); } - var shaperRemapper = new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset); + // Inner should not have pendingCollection since we apply them first. + // Shaper should not have CollectionShaperExpression as any collection would get converted to RelationalCollectionShaperExpression. + var shaperRemapper = new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset, pendingCollectionOffset: 0); innerShaper = shaperRemapper.Visit(innerShaper); selfIdentifier = shaperRemapper.Visit(selfIdentifier); @@ -1014,43 +1018,54 @@ private sealed class ShaperRemappingExpressionVisitor : ExpressionVisitor { private readonly SelectExpression _queryExpression; private readonly SelectExpression _innerSelectExpression; - private readonly int _offset; + private readonly int _projectionOffset; + private readonly int _pendingCollectionOffset; - public ShaperRemappingExpressionVisitor(SelectExpression queryExpression, SelectExpression innerSelectExpression, int offset) + public ShaperRemappingExpressionVisitor( + SelectExpression queryExpression, SelectExpression innerSelectExpression, int projectionOffset, int pendingCollectionOffset) { _queryExpression = queryExpression; _innerSelectExpression = innerSelectExpression; - _offset = offset; + _projectionOffset = projectionOffset; + _pendingCollectionOffset = pendingCollectionOffset; } protected override Expression VisitExtension(Expression extensionExpression) { Check.NotNull(extensionExpression, nameof(extensionExpression)); - if (extensionExpression is ProjectionBindingExpression projectionBindingExpression) + switch (extensionExpression) { - var oldIndex = (int)GetProjectionIndex(projectionBindingExpression); + case ProjectionBindingExpression projectionBindingExpression: + var oldIndex = (int)GetProjectionIndex(projectionBindingExpression); - return new ProjectionBindingExpression(_queryExpression, oldIndex + _offset, projectionBindingExpression.Type); - } + return new ProjectionBindingExpression( + _queryExpression, oldIndex + _projectionOffset, projectionBindingExpression.Type); - if (extensionExpression is EntityShaperExpression entityShaper) - { - var oldIndexMap = (IDictionary)GetProjectionIndex( - (ProjectionBindingExpression)entityShaper.ValueBufferExpression); - var indexMap = new Dictionary(); - foreach (var keyValuePair in oldIndexMap) - { - indexMap[keyValuePair.Key] = keyValuePair.Value + _offset; - } + case EntityShaperExpression entityShaperExpression: + var oldIndexMap = (IDictionary)GetProjectionIndex( + (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression); + var indexMap = new Dictionary(); + foreach (var keyValuePair in oldIndexMap) + { + indexMap[keyValuePair.Key] = keyValuePair.Value + _projectionOffset; + } - return new EntityShaperExpression( - entityShaper.EntityType, - new ProjectionBindingExpression(_queryExpression, indexMap), - nullable: true); - } + return new EntityShaperExpression( + entityShaperExpression.EntityType, + new ProjectionBindingExpression(_queryExpression, indexMap), + nullable: true); + + case CollectionShaperExpression collectionShaperExpression: + var collectionIndex = (int)GetProjectionIndex((ProjectionBindingExpression)collectionShaperExpression.Projection); + + return collectionShaperExpression.Update( + new ProjectionBindingExpression(_queryExpression, collectionIndex + _pendingCollectionOffset, typeof(object)), + collectionShaperExpression.InnerShaper); - return base.VisitExtension(extensionExpression); + default: + return base.VisitExtension(extensionExpression); + } } private object GetProjectionIndex(ProjectionBindingExpression projectionBindingExpression) @@ -1363,6 +1378,9 @@ private void AddJoin( } var innerTable = innerSelectExpression.Tables.Single(); + // Copy over pending collection if in join else that info would be lost. + _pendingCollections.AddRange(innerSelectExpression._pendingCollections); + var joinTable = joinType switch { JoinType.InnerJoin => new InnerJoinExpression(innerTable, joinPredicate), diff --git a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs index c2acdbb3e86..eca74b677fe 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs @@ -32,5 +32,17 @@ public override Task Lift_projection_mapping_when_pushing_down_subquery(bool asy { return base.Lift_projection_mapping_when_pushing_down_subquery(async); } + + [ConditionalTheory(Skip = "issue #19344")] + public override Task Select_subquery_single_nested_subquery(bool async) + { + return base.Select_subquery_single_nested_subquery(async); + } + + [ConditionalTheory(Skip = "issue #19344")] + public override Task Select_subquery_single_nested_subquery2(bool async) + { + return base.Select_subquery_single_nested_subquery2(async); + } } } diff --git a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs index d41594d9e82..6326c059a61 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs @@ -59,5 +59,17 @@ public override Task OrderBy_collection_count_ThenBy_reference_navigation(bool a { return base.OrderBy_collection_count_ThenBy_reference_navigation(async); } + + [ConditionalTheory(Skip = "issue #19344")] + public override Task Select_subquery_single_nested_subquery(bool async) + { + return base.Select_subquery_single_nested_subquery(async); + } + + [ConditionalTheory(Skip = "issue #19344")] + public override Task Select_subquery_single_nested_subquery2(bool async) + { + return base.Select_subquery_single_nested_subquery2(async); + } } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs index 59705f205e3..8aec7e573ae 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs @@ -5616,5 +5616,66 @@ public virtual Task Select_with_joined_where_clause_cast_using_as(bool async) ss => ss.Set() .Where(w => w.Id == (MaybeScalar(w.OneToOne_Optional_FK1, () => w.OneToOne_Optional_FK1.Id) as int?))); } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Select_subquery_single_nested_subquery(bool async) + { + return AssertQuery( + async, + ss => ss.Set().OrderBy(l1 => l1.Id).Select(l1 => new + { + Level2 = l1.OneToMany_Optional1.OrderBy(l2 => l2.Id).Select(l2 => new + { + Level3s = l2.OneToMany_Optional2.OrderBy(l3 => l3.Id).Select(l3 => new { l3.Id }).ToList() + }).FirstOrDefault() + }), + assertOrder: true, + elementAsserter: (e, a) => + { + if (e.Level2 == null) + { + Assert.Null(a.Level2); + } + else + { + AssertCollection(e.Level2.Level3s, a.Level2.Level3s, ordered: true); + } + }); + + } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Select_subquery_single_nested_subquery2(bool async) + { + return AssertQuery( + async, + ss => ss.Set().OrderBy(l1 => l1.Id).Select(l1 => new + { + Level2s = l1.OneToMany_Optional1.OrderBy(l2 => l2.Id).Select(l2 => new + { + Level3 = l2.OneToMany_Optional2.OrderBy(l3 => l3.Id).Select(l3 => new + { + Level4s = l3.OneToMany_Optional3.OrderBy(l4 => l4.Id).Select(l4 => new { l4.Id }).ToList() + }).FirstOrDefault() + }) + }), + assertOrder: true, + elementAsserter: (e, a) => + { + AssertCollection(e.Level2s, a.Level2s, ordered: true, elementAsserter: + (e2, a2) => + { + if (e2.Level3 == null) + { + Assert.Null(a2.Level3); + } + else + { + AssertCollection(e2.Level3.Level4s, a2.Level3.Level4s, ordered: true); + } + }); + }); + } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs index 8dfc81f496f..525853ef2e3 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs @@ -4363,6 +4363,48 @@ FROM [LevelOne] AS [l] WHERE [l].[Id] = [l0].[Id]"); } + public override async Task Select_subquery_single_nested_subquery(bool async) + { + await base.Select_subquery_single_nested_subquery(async); + + AssertSql( + @"SELECT [t0].[c], [l].[Id], [l1].[Id] +FROM [LevelOne] AS [l] +LEFT JOIN ( + SELECT [t].[c], [t].[Id], [t].[OneToMany_Optional_Inverse2Id] + FROM ( + SELECT 1 AS [c], [l0].[Id], [l0].[OneToMany_Optional_Inverse2Id], ROW_NUMBER() OVER(PARTITION BY [l0].[OneToMany_Optional_Inverse2Id] ORDER BY [l0].[Id]) AS [row] + FROM [LevelTwo] AS [l0] + ) AS [t] + WHERE [t].[row] <= 1 +) AS [t0] ON [l].[Id] = [t0].[OneToMany_Optional_Inverse2Id] +LEFT JOIN [LevelThree] AS [l1] ON [t0].[Id] = [l1].[OneToMany_Optional_Inverse3Id] +ORDER BY [l].[Id], [l1].[Id]"); + } + + public override async Task Select_subquery_single_nested_subquery2(bool async) + { + await base.Select_subquery_single_nested_subquery2(async); + + AssertSql( + @"SELECT [l].[Id], [t1].[c], [t1].[Id], [t1].[Id0] +FROM [LevelOne] AS [l] +LEFT JOIN ( + SELECT [t0].[c], [l0].[Id], [l2].[Id] AS [Id0], [l0].[OneToMany_Optional_Inverse2Id] + FROM [LevelTwo] AS [l0] + LEFT JOIN ( + SELECT [t].[c], [t].[Id], [t].[OneToMany_Optional_Inverse3Id] + FROM ( + SELECT 1 AS [c], [l1].[Id], [l1].[OneToMany_Optional_Inverse3Id], ROW_NUMBER() OVER(PARTITION BY [l1].[OneToMany_Optional_Inverse3Id] ORDER BY [l1].[Id]) AS [row] + FROM [LevelThree] AS [l1] + ) AS [t] + WHERE [t].[row] <= 1 + ) AS [t0] ON [l0].[Id] = [t0].[OneToMany_Optional_Inverse3Id] + LEFT JOIN [LevelFour] AS [l2] ON [t0].[Id] = [l2].[OneToMany_Optional_Inverse4Id] +) AS [t1] ON [l].[Id] = [t1].[OneToMany_Optional_Inverse2Id] +ORDER BY [l].[Id], [t1].[Id], [t1].[Id0]"); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } }