diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs index 644dd171dce..612a64d9500 100644 --- a/src/EFCore.Relational/Query/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/QuerySqlGenerator.cs @@ -44,6 +44,9 @@ public class QuerySqlGenerator : SqlExpressionVisitor private static readonly bool UseOldBehavior32375 = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32375", out var enabled32375) && enabled32375; + private static readonly bool UseOldBehavior36105 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36105", out var enabled36105) && enabled36105; + /// /// Creates a new instance of the class. /// @@ -1276,9 +1279,16 @@ static string GetSetOperation(SetOperationBase operation) protected virtual void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand) { // INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right. - // To preserve meaning, add parentheses whenever a set operation is nested within a different set operation. + // To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation + // - including different distinctness. + // In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105). if (IsNonComposedSetOperation(operand) - && operand.Tables[0].GetType() != setOperation.GetType()) + && ((UseOldBehavior36105 && operand.Tables[0].GetType() != setOperation.GetType()) + || (!UseOldBehavior36105 + && operand.Tables[0] is SetOperationBase nestedSetOperation + && (nestedSetOperation is ExceptExpression + || nestedSetOperation.GetType() != setOperation.GetType() + || nestedSetOperation.IsDistinct != setOperation.IsDistinct)))) { _relationalCommandBuilder.AppendLine("("); using (_relationalCommandBuilder.Indent()) diff --git a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs index 3868ed9ae49..db95ce7c2da 100644 --- a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics.CodeAnalysis; using Microsoft.EntityFrameworkCore.Query.SqlExpressions; using Microsoft.EntityFrameworkCore.Sqlite.Query.SqlExpressions.Internal; @@ -14,6 +15,9 @@ namespace Microsoft.EntityFrameworkCore.Sqlite.Query.Internal; /// public class SqliteQuerySqlGenerator : QuerySqlGenerator { + private static readonly bool UseOldBehavior36112 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36112", out var enabled36112) && enabled36112; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in @@ -98,8 +102,63 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression) /// doing so can result in application failures when updating to a new Entity Framework Core release. /// protected override void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand) - // Sqlite doesn't support parentheses around set operation operands - => Visit(operand); + { + // Most databases support parentheses around set operations to determine evaluation order, but SQLite does not; + // however, we can instead wrap the nested set operation in a SELECT * FROM () to achieve the same effect. + // The following is a copy-paste of the base implementation from QuerySqlGenerator, adding the SELECT. + + // INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right. + // To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation + // - including different distinctness. + // In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105). + if (!UseOldBehavior36112 + && TryUnwrapBareSetOperation(operand, out var nestedSetOperation) + && (nestedSetOperation is ExceptExpression + || nestedSetOperation.GetType() != setOperation.GetType() + || nestedSetOperation.IsDistinct != setOperation.IsDistinct)) + { + Sql.AppendLine("SELECT * FROM ("); + + using (Sql.Indent()) + { + Visit(operand); + } + + Sql.AppendLine().Append(")"); + } + else + { + Visit(operand); + } + + static bool TryUnwrapBareSetOperation(SelectExpression selectExpression, [NotNullWhen(true)] out SetOperationBase? setOperation) + { + if (selectExpression is + { + Tables: [SetOperationBase s], + Predicate: null, + Orderings: [], + Offset: null, + Limit: null, + IsDistinct: false, + Having: null, + GroupBy: [] + } + && selectExpression.Projection.Count == s.Source1.Projection.Count + && selectExpression.Projection.Select( + (pe, index) => pe.Expression is ColumnExpression column + && column.TableAlias == s.Alias + && column.Name == s.Source1.Projection[index].Alias) + .All(e => e)) + { + setOperation = s; + return true; + } + + setOperation = null; + return false; + } + } private void GenerateGlob(GlobExpression globExpression, bool negated = false) { diff --git a/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs index a6d4ea8d07d..72992cfb49c 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs @@ -82,6 +82,19 @@ public virtual Task Except_nested(bool async) .Except(ss.Set().Where(s => s.City == "México D.F.")) .Except(ss.Set().Where(e => e.City == "Seattle"))); + // EXCEPT is non-commutative, unlike UNION/INTERSECT. Therefore, parentheses are needed in the following query + // to ensure that the inner EXCEPT is evaluated first. See #36105. + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Except_nested2(bool async) + => AssertQuery( + async, + ss => ss.Set() + .Except(ss.Set() + .Where(s => s.City == "Seattle") + .Except(ss.Set() + .Where(e => e.City == "Seattle")))); + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Except_non_entity(bool async) @@ -221,6 +234,17 @@ public virtual Task Union_Intersect(bool async) .Union(ss.Set().Where(c => c.City == "London")) .Intersect(ss.Set().Where(c => c.ContactName.Contains("Thomas")))); + // The evaluation order of Concat and Union can matter: A UNION ALL (B UNION C) can be different from (A UNION ALL B) UNION C. + // Make sure parentheses are added. + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Union_inside_Concat(bool async) + => AssertQuery( + async, + ss => ss.Set().Where(c => c.City == "Berlin") + .Concat(ss.Set().Where(c => c.City == "London") + .Union(ss.Set().Where(c => c.City == "Berlin")))); + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Union_Take_Union_Take(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs index ded5240008b..e010573267d 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs @@ -201,6 +201,28 @@ WHERE [c1].[ContactName] LIKE N'%Thomas%' """); } + public override async Task Union_inside_Concat(bool async) + { + await base.Union_inside_Concat(async); + +AssertSql( +""" +SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] +FROM [Customers] AS [c] +WHERE [c].[City] = N'Berlin' +UNION ALL +( + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'London' + UNION + SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] + FROM [Customers] AS [c1] + WHERE [c1].[City] = N'Berlin' +) +"""); + } + public override async Task Union_Take_Union_Take(bool async) { await base.Union_Take_Union_Take(async); @@ -1234,14 +1256,16 @@ public override async Task Except_nested(bool async) await base.Except_nested(async); AssertSql( - """ -SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] -FROM [Customers] AS [c] -WHERE [c].[ContactTitle] = N'Owner' -EXCEPT -SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] -FROM [Customers] AS [c0] -WHERE [c0].[City] = N'México D.F.' +""" +( + SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] + FROM [Customers] AS [c] + WHERE [c].[ContactTitle] = N'Owner' + EXCEPT + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'México D.F.' +) EXCEPT SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] FROM [Customers] AS [c1] @@ -1249,6 +1273,27 @@ FROM [Customers] AS [c1] """); } + public override async Task Except_nested2(bool async) + { + await base.Except_nested2(async); + + AssertSql( +""" +SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] +FROM [Customers] AS [c] +EXCEPT +( + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'Seattle' + EXCEPT + SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] + FROM [Customers] AS [c1] + WHERE [c1].[City] = N'Seattle' +) +"""); + } + public override async Task Intersect_non_entity(bool async) { await base.Intersect_non_entity(async);