From 6579020d286af0787fcd347660f4b348197d9efb Mon Sep 17 00:00:00 2001 From: maumar Date: Wed, 18 Oct 2023 23:38:37 -0700 Subject: [PATCH] Fix to #32023 - Case expressions may only be nested to level 10 when using SplitQuery() or ToList() inside projection. Problem was that our SqlExpression simplifying visitor, when visiting ShapedQuery would only look into QueryExpression part. This makes sense in general as QueryExpression is where the SqlExpression is stored. However, for split query scenarios, we also have SqlExpression in the ShaperExpression (wrapped by RelationalSplitCollectionShaperExpression), and all the optimizations were not run on that SqlExpression piece, which in that case caused too deep CASE WHEN ELSE structure (that we normally optimize into flat CASE with multiple WHENs. Fix is to also visit ShaperExpression when performing post processing. Fixes #32023 --- ...eConverterCompensatingExpressionVisitor.cs | 7 +++- ...lExpressionSimplifyingExpressionVisitor.cs | 5 ++- ...RelationalQueryTranslationPostprocessor.cs | 1 + ...plexNavigationsCollectionsQueryTestBase.cs | 23 ++++++++++++ ...avigationsCollectionsQuerySqlServerTest.cs | 24 +++++++++++++ ...CollectionsSharedTypeQuerySqlServerTest.cs | 30 ++++++++++++++++ ...tionsCollectionsSplitQuerySqlServerTest.cs | 36 +++++++++++++++++++ ...avigationsCollectionsQuerySqlServerTest.cs | 24 +++++++++++++ 8 files changed, 148 insertions(+), 2 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs index b82037f6aa0..41ead38049d 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalValueConverterCompensatingExpressionVisitor.cs @@ -46,7 +46,12 @@ protected override Expression VisitExtension(Expression extensionExpression) }; private Expression VisitShapedQueryExpression(ShapedQueryExpression shapedQueryExpression) - => shapedQueryExpression.UpdateQueryExpression(Visit(shapedQueryExpression.QueryExpression)); + { + var newQueryExpression = Visit(shapedQueryExpression.QueryExpression); + var newShaperExpression = Visit(shapedQueryExpression.ShaperExpression); + + return shapedQueryExpression.Update(newQueryExpression, newShaperExpression); + } private Expression VisitCase(CaseExpression caseExpression) { diff --git a/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs index 9a2e955f2bd..a252cdf6ff9 100644 --- a/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs @@ -39,7 +39,10 @@ protected override Expression VisitExtension(Expression extensionExpression) { if (extensionExpression is ShapedQueryExpression shapedQueryExpression) { - return shapedQueryExpression.UpdateQueryExpression(Visit(shapedQueryExpression.QueryExpression)); + var newQueryExpression = Visit(shapedQueryExpression.QueryExpression); + var newShaperExpression = Visit(shapedQueryExpression.ShaperExpression); + + return shapedQueryExpression.Update(newQueryExpression, newShaperExpression); } // Only applies to 'CASE WHEN condition...' not 'CASE operand WHEN...' diff --git a/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs b/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs index 837180ceabb..c166dbcc10f 100644 --- a/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs +++ b/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs @@ -69,6 +69,7 @@ private sealed class SelectExpressionMutableVerifyingExpressionVisitor : Express case ShapedQueryExpression shapedQueryExpression: Visit(shapedQueryExpression.QueryExpression); + Visit(shapedQueryExpression.ShaperExpression); return shapedQueryExpression; default: diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsCollectionsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsCollectionsQueryTestBase.cs index f9847c25d67..1fed4b93141 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsCollectionsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsCollectionsQueryTestBase.cs @@ -2530,4 +2530,27 @@ public virtual Task Final_GroupBy_property_entity_Include_reference_multiple(boo ee, aa, new ExpectedInclude(l2 => l2.OneToOne_Optional_FK1), new ExpectedInclude(l2 => l2.OneToOne_Required_FK1)))); + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Project_collection_and_nested_conditional(bool async) + => AssertQuery( + async, + ss => ss.Set().OrderBy(x => x.Id).Select(x => new + { + Collection = x.OneToMany_Optional1.OrderBy(xx => xx.Id).Select(xx => xx.Name).ToList(), + Condition = x.Id == 1 + ? "01" + : x.Id == 2 + ? "02" + : x.Id == 3 + ? "03" + : null + }).Where(x => x.Condition == "02"), + assertOrder: true, + elementAsserter: (e, a) => + { + AssertCollection(e.Collection, a.Collection, ordered: true); + AssertEqual(e.Condition, a.Condition); + }); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsQuerySqlServerTest.cs index 2646c722d4b..52fc8c74eb3 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsQuerySqlServerTest.cs @@ -2978,6 +2978,30 @@ ORDER BY [l].[Name] """); } + public override async Task Project_collection_and_nested_conditional(bool async) + { + await base.Project_collection_and_nested_conditional(async); + + AssertSql( +""" +SELECT [l].[Id], [l0].[Name], [l0].[Id], CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END +FROM [LevelOne] AS [l] +LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id] +WHERE CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END = N'02' +ORDER BY [l].[Id], [l0].[Id] +"""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSharedTypeQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSharedTypeQuerySqlServerTest.cs index e614eae9108..c2b2b3a5e68 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSharedTypeQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSharedTypeQuerySqlServerTest.cs @@ -3870,6 +3870,36 @@ ORDER BY [l].[Name] """); } + public override async Task Project_collection_and_nested_conditional(bool async) + { + await base.Project_collection_and_nested_conditional(async); + + AssertSql( +""" +SELECT [l].[Id], [t].[Level2_Name], [t].[Id], CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END +FROM [Level1] AS [l] +LEFT JOIN ( + SELECT [l0].[Level2_Name], [l0].[Id], CASE + WHEN [l0].[OneToOne_Required_PK_Date] IS NOT NULL AND [l0].[Level1_Required_Id] IS NOT NULL AND [l0].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l0].[Id] + END AS [c], [l0].[OneToMany_Optional_Inverse2Id] + FROM [Level1] AS [l0] + WHERE [l0].[OneToOne_Required_PK_Date] IS NOT NULL AND [l0].[Level1_Required_Id] IS NOT NULL AND [l0].[OneToMany_Required_Inverse2Id] IS NOT NULL +) AS [t] ON [l].[Id] = [t].[OneToMany_Optional_Inverse2Id] +WHERE CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END = N'02' +ORDER BY [l].[Id], [t].[c] +"""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs index 15c25223ab6..344d9531f97 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs @@ -4413,6 +4413,42 @@ ORDER BY [l].[Name] """); } + public override async Task Project_collection_and_nested_conditional(bool async) + { + await base.Project_collection_and_nested_conditional(async); + + AssertSql( +""" +SELECT [l].[Id], CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END +FROM [LevelOne] AS [l] +WHERE CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END = N'02' +ORDER BY [l].[Id] +""", + // + """ +SELECT [l0].[Name], [l].[Id] +FROM [LevelOne] AS [l] +INNER JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id] +WHERE CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END = N'02' +ORDER BY [l].[Id], [l0].[Id] +"""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TemporalComplexNavigationsCollectionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TemporalComplexNavigationsCollectionsQuerySqlServerTest.cs index fa79096a1fd..7c1c0462aee 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/TemporalComplexNavigationsCollectionsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/TemporalComplexNavigationsCollectionsQuerySqlServerTest.cs @@ -2998,6 +2998,30 @@ ORDER BY [l].[Name] """); } + public override async Task Project_collection_and_nested_conditional(bool async) + { + await base.Project_collection_and_nested_conditional(async); + + AssertSql( +""" +SELECT [l].[Id], [l0].[Name], [l0].[Id], CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END +FROM [LevelOne] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [l] +LEFT JOIN [LevelTwo] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [l0] ON [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id] +WHERE CASE + WHEN [l].[Id] = 1 THEN N'01' + WHEN [l].[Id] = 2 THEN N'02' + WHEN [l].[Id] = 3 THEN N'03' + ELSE NULL +END = N'02' +ORDER BY [l].[Id], [l0].[Id] +"""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); }