From 4cbc5d79b4df0fb999a502d20f586f0aefd5a0fa Mon Sep 17 00:00:00 2001 From: Maurycy Markowski Date: Tue, 11 Feb 2020 11:22:05 -0800 Subject: [PATCH] Fix to #19825 - Query: incorrectly generating JOIN rather than APPLY for subqueries with outside references to a joined table During translation, when we decide whether we can apply JOIN or APPLY we visit inner SelectExpression looking for references to tables in the outer SelectExpression. Problem was that we were using Contains method, which would not match cases where the outer table was in the form of JoinExpressionBase. Fix is to use ContainsTableReference method which matches different types of tables. --- .../Query/SqlExpressions/SelectExpression.cs | 12 ++++----- ...ComplexNavigationsWeakQueryInMemoryTest.cs | 12 +++++++++ .../Query/ComplexNavigationsQueryTestBase.cs | 27 +++++++++++++++++++ .../ComplexNavigationsQuerySqlServerTest.cs | 25 +++++++++++++++++ .../ComplexNavigationsQuerySqliteTest.cs | 4 +++ .../ComplexNavigationsWeakQuerySqliteTest.cs | 4 +++ 6 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 68f247d2aa3..7589665d3d1 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -938,7 +938,7 @@ public Expression ApplyCollectionJoin( } var joinPredicate = TryExtractJoinKey(innerSelectExpression); - var containsOuterReference = new SelectExpressionCorrelationFindingExpressionVisitor(Tables) + var containsOuterReference = new SelectExpressionCorrelationFindingExpressionVisitor(this) .ContainsOuterReference(innerSelectExpression); if (containsOuterReference && joinPredicate != null) { @@ -1226,12 +1226,12 @@ private bool ContainsTableReference(TableExpressionBase table) private sealed class SelectExpressionCorrelationFindingExpressionVisitor : ExpressionVisitor { - private readonly IReadOnlyList _tables; + private readonly SelectExpression _outerSelectExpression; private bool _containsOuterReference; - public SelectExpressionCorrelationFindingExpressionVisitor(IReadOnlyList tables) + public SelectExpressionCorrelationFindingExpressionVisitor(SelectExpression outerSelectExpression) { - _tables = tables; + _outerSelectExpression = outerSelectExpression; } public bool ContainsOuterReference(SelectExpression selectExpression) @@ -1251,7 +1251,7 @@ public override Expression Visit(Expression expression) } if (expression is ColumnExpression columnExpression - && _tables.Contains(columnExpression.Table)) + && _outerSelectExpression.ContainsTableReference(columnExpression.Table)) { _containsOuterReference = true; @@ -1304,7 +1304,7 @@ private void AddJoin( joinPredicate = TryExtractJoinKey(innerSelectExpression); if (joinPredicate != null) { - var containsOuterReference = new SelectExpressionCorrelationFindingExpressionVisitor(Tables) + var containsOuterReference = new SelectExpressionCorrelationFindingExpressionVisitor(this) .ContainsOuterReference(innerSelectExpression); if (containsOuterReference) { diff --git a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs index 6326c059a61..69b8ceab94d 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs @@ -71,5 +71,17 @@ public override Task Select_subquery_single_nested_subquery2(bool async) { return base.Select_subquery_single_nested_subquery2(async); } + + [ConditionalTheory(Skip = "issue #19967")] + public override Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async) + { + return base.SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(async); + } + + [ConditionalTheory(Skip = "issue #19967")] + public override Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async) + { + return base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async); + } } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs index 5a99893253e..b2a47ae54d7 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs @@ -5164,6 +5164,7 @@ public virtual Task Select_subquery_single_nested_subquery(bool async) }); } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Select_subquery_single_nested_subquery2(bool async) @@ -5197,5 +5198,31 @@ public virtual Task Select_subquery_single_nested_subquery2(bool async) }); }); } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async) + { + return AssertQuery( + async, + ss => from l1 in ss.Set() + join l2 in ss.Set() on l1.Id equals l2.Level1_Required_Id + join l3 in ss.Set() on l2.Id equals l3.Level2_Required_Id + join l4 in ss.Set() on l3.Id equals l4.Level3_Required_Id + from other in ss.Set().Where(x => x.Id <= l2.Id && x.Name == l4.Name).DefaultIfEmpty() + select l1); + } + + [ConditionalTheory(Skip = "issue #19095")] + [MemberData(nameof(IsAsyncData))] + public virtual Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async) + { + return AssertQuery( + async, + ss => ss.Set().SelectMany( + l1 => l1.OneToMany_Optional1.DefaultIfEmpty().SelectMany( + l2 => l2.OneToOne_Required_PK2.OneToMany_Optional3.DefaultIfEmpty() + .Select(l4 => new { l1Name = l1.Name, l2Name = l2.OneToOne_Required_PK2.Name, l3Name = l4.OneToOne_Optional_PK_Inverse4.Name })))); + } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs index 589f8cc15e1..0ff6dac3d5b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs @@ -4398,6 +4398,31 @@ FROM [LevelThree] AS [l1] ORDER BY [l].[Id], [t1].[Id], [t1].[Id0]"); } + public override async Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async) + { + await base.SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(async); + + AssertSql( + @"SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id] +FROM [LevelOne] AS [l] +INNER JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Required_Id] +INNER JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Level2_Required_Id] +INNER JOIN [LevelFour] AS [l2] ON [l1].[Id] = [l2].[Level3_Required_Id] +OUTER APPLY ( + SELECT [l3].[Id], [l3].[Date], [l3].[Name], [l3].[OneToMany_Optional_Self_Inverse1Id], [l3].[OneToMany_Required_Self_Inverse1Id], [l3].[OneToOne_Optional_Self1Id] + FROM [LevelOne] AS [l3] + WHERE ([l3].[Id] <= [l0].[Id]) AND (([l2].[Name] = [l3].[Name]) OR ([l2].[Name] IS NULL AND [l3].[Name] IS NULL)) +) AS [t]"); + } + + public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async) + { + await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async); + + AssertSql( + @""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } } diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsQuerySqliteTest.cs index c29d5dde5a7..f82f06b6a3c 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsQuerySqliteTest.cs @@ -30,5 +30,9 @@ public override Task Include_inside_subquery(bool async) { return base.Include_inside_subquery(async); } + + // Sqlite does not support cross/outer apply + public override Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async) => null; + public override Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async) => null; } } diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs index 01ab90a0bdd..b33bff236a4 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs @@ -25,5 +25,9 @@ public override Task Project_collection_navigation_nested_with_take(bool async) { return base.Project_collection_navigation_nested_with_take(async); } + + // Sqlite does not support cross/outer apply + public override Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async) => null; + public override Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async) => null; } }