Skip to content

Commit ccf6fab

Browse files
rojiroji
authored
Add missing parentheses for set operations (#36138)
Fixes #36105 Fixes #36112
1 parent 015f170 commit ccf6fab

4 files changed

Lines changed: 150 additions & 12 deletions

File tree

‎src/EFCore.Relational/Query/QuerySqlGenerator.cs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ public class QuerySqlGenerator : SqlExpressionVisitor
2222
private IRelationalCommandBuilder _relationalCommandBuilder;
2323
private Dictionary<string, int>? _repeatedParameterCounts;
2424

25+
private static readonly bool UseOldBehavior36105 =
26+
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36105", out var enabled36105) && enabled36105;
27+
2528
/// <summary>
2629
/// Creates a new instance of the <see cref="QuerySqlGenerator" /> class.
2730
/// </summary>
@@ -1382,9 +1385,16 @@ static string GetSetOperation(SetOperationBase operation)
13821385
protected virtual void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand)
13831386
{
13841387
// INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right.
1385-
// To preserve meaning, add parentheses whenever a set operation is nested within a different set operation.
1388+
// To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation
1389+
// - including different distinctness.
1390+
// In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105).
13861391
if (IsNonComposedSetOperation(operand)
1387-
&& operand.Tables[0].GetType() != setOperation.GetType())
1392+
&& ((UseOldBehavior36105 && operand.Tables[0].GetType() != setOperation.GetType())
1393+
|| (!UseOldBehavior36105
1394+
&& operand.Tables[0] is SetOperationBase nestedSetOperation
1395+
&& (nestedSetOperation is ExceptExpression
1396+
|| nestedSetOperation.GetType() != setOperation.GetType()
1397+
|| nestedSetOperation.IsDistinct != setOperation.IsDistinct))))
13881398
{
13891399
_relationalCommandBuilder.AppendLine("(");
13901400
using (_relationalCommandBuilder.Indent())

‎src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Diagnostics.CodeAnalysis;
45
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
56

67
namespace Microsoft.EntityFrameworkCore.Sqlite.Query.Internal;
@@ -13,6 +14,9 @@ namespace Microsoft.EntityFrameworkCore.Sqlite.Query.Internal;
1314
/// </summary>
1415
public class SqliteQuerySqlGenerator : QuerySqlGenerator
1516
{
17+
private static readonly bool UseOldBehavior36112 =
18+
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36112", out var enabled36112) && enabled36112;
19+
1620
/// <summary>
1721
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
1822
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -97,8 +101,63 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression)
97101
/// doing so can result in application failures when updating to a new Entity Framework Core release.
98102
/// </summary>
99103
protected override void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand)
100-
// Sqlite doesn't support parentheses around set operation operands
101-
=> Visit(operand);
104+
{
105+
// Most databases support parentheses around set operations to determine evaluation order, but SQLite does not;
106+
// however, we can instead wrap the nested set operation in a SELECT * FROM () to achieve the same effect.
107+
// The following is a copy-paste of the base implementation from QuerySqlGenerator, adding the SELECT.
108+
109+
// INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right.
110+
// To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation
111+
// - including different distinctness.
112+
// In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105).
113+
if (!UseOldBehavior36112
114+
&& TryUnwrapBareSetOperation(operand, out var nestedSetOperation)
115+
&& (nestedSetOperation is ExceptExpression
116+
|| nestedSetOperation.GetType() != setOperation.GetType()
117+
|| nestedSetOperation.IsDistinct != setOperation.IsDistinct))
118+
{
119+
Sql.AppendLine("SELECT * FROM (");
120+
121+
using (Sql.Indent())
122+
{
123+
Visit(operand);
124+
}
125+
126+
Sql.AppendLine().Append(")");
127+
}
128+
else
129+
{
130+
Visit(operand);
131+
}
132+
133+
static bool TryUnwrapBareSetOperation(SelectExpression selectExpression, [NotNullWhen(true)] out SetOperationBase? setOperation)
134+
{
135+
if (selectExpression is
136+
{
137+
Tables: [SetOperationBase s],
138+
Predicate: null,
139+
Orderings: [],
140+
Offset: null,
141+
Limit: null,
142+
IsDistinct: false,
143+
Having: null,
144+
GroupBy: []
145+
}
146+
&& selectExpression.Projection.Count == s.Source1.Projection.Count
147+
&& selectExpression.Projection.Select(
148+
(pe, index) => pe.Expression is ColumnExpression column
149+
&& column.TableAlias == s.Alias
150+
&& column.Name == s.Source1.Projection[index].Alias)
151+
.All(e => e))
152+
{
153+
setOperation = s;
154+
return true;
155+
}
156+
157+
setOperation = null;
158+
return false;
159+
}
160+
}
102161

103162
private void GenerateGlob(GlobExpression globExpression, bool negated = false)
104163
{

‎test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,19 @@ public virtual Task Except_nested(bool async)
7979
.Except(ss.Set<Customer>().Where(s => s.City == "México D.F."))
8080
.Except(ss.Set<Customer>().Where(e => e.City == "Seattle")));
8181

82+
// EXCEPT is non-commutative, unlike UNION/INTERSECT. Therefore, parentheses are needed in the following query
83+
// to ensure that the inner EXCEPT is evaluated first. See #36105.
84+
[ConditionalTheory]
85+
[MemberData(nameof(IsAsyncData))]
86+
public virtual Task Except_nested2(bool async)
87+
=> AssertQuery(
88+
async,
89+
ss => ss.Set<Customer>()
90+
.Except(ss.Set<Customer>()
91+
.Where(s => s.City == "Seattle")
92+
.Except(ss.Set<Customer>()
93+
.Where(e => e.City == "Seattle"))));
94+
8295
[ConditionalTheory]
8396
[MemberData(nameof(IsAsyncData))]
8497
public virtual Task Except_non_entity(bool async)
@@ -218,6 +231,17 @@ public virtual Task Union_Intersect(bool async)
218231
.Union(ss.Set<Customer>().Where(c => c.City == "London"))
219232
.Intersect(ss.Set<Customer>().Where(c => c.ContactName.Contains("Thomas"))));
220233

234+
// 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.
235+
// Make sure parentheses are added.
236+
[ConditionalTheory]
237+
[MemberData(nameof(IsAsyncData))]
238+
public virtual Task Union_inside_Concat(bool async)
239+
=> AssertQuery(
240+
async,
241+
ss => ss.Set<Customer>().Where(c => c.City == "Berlin")
242+
.Concat(ss.Set<Customer>().Where(c => c.City == "London")
243+
.Union(ss.Set<Customer>().Where(c => c.City == "Berlin"))));
244+
221245
[ConditionalTheory]
222246
[MemberData(nameof(IsAsyncData))]
223247
public virtual Task Union_Take_Union_Take(bool async)

‎test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs‎

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,28 @@ WHERE [c1].[ContactName] LIKE N'%Thomas%'
200200
""");
201201
}
202202

203+
public override async Task Union_inside_Concat(bool async)
204+
{
205+
await base.Union_inside_Concat(async);
206+
207+
AssertSql(
208+
"""
209+
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
210+
FROM [Customers] AS [c]
211+
WHERE [c].[City] = N'Berlin'
212+
UNION ALL
213+
(
214+
SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
215+
FROM [Customers] AS [c0]
216+
WHERE [c0].[City] = N'London'
217+
UNION
218+
SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region]
219+
FROM [Customers] AS [c1]
220+
WHERE [c1].[City] = N'Berlin'
221+
)
222+
""");
223+
}
224+
203225
public override async Task Union_Take_Union_Take(bool async)
204226
{
205227
await base.Union_Take_Union_Take(async);
@@ -1233,21 +1255,44 @@ public override async Task Except_nested(bool async)
12331255
await base.Except_nested(async);
12341256

12351257
AssertSql(
1236-
"""
1237-
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
1238-
FROM [Customers] AS [c]
1239-
WHERE [c].[ContactTitle] = N'Owner'
1240-
EXCEPT
1241-
SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
1242-
FROM [Customers] AS [c0]
1243-
WHERE [c0].[City] = N'México D.F.'
1258+
"""
1259+
(
1260+
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
1261+
FROM [Customers] AS [c]
1262+
WHERE [c].[ContactTitle] = N'Owner'
1263+
EXCEPT
1264+
SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
1265+
FROM [Customers] AS [c0]
1266+
WHERE [c0].[City] = N'México D.F.'
1267+
)
12441268
EXCEPT
12451269
SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region]
12461270
FROM [Customers] AS [c1]
12471271
WHERE [c1].[City] = N'Seattle'
12481272
""");
12491273
}
12501274

1275+
public override async Task Except_nested2(bool async)
1276+
{
1277+
await base.Except_nested2(async);
1278+
1279+
AssertSql(
1280+
"""
1281+
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
1282+
FROM [Customers] AS [c]
1283+
EXCEPT
1284+
(
1285+
SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
1286+
FROM [Customers] AS [c0]
1287+
WHERE [c0].[City] = N'Seattle'
1288+
EXCEPT
1289+
SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region]
1290+
FROM [Customers] AS [c1]
1291+
WHERE [c1].[City] = N'Seattle'
1292+
)
1293+
""");
1294+
}
1295+
12511296
public override async Task Intersect_non_entity(bool async)
12521297
{
12531298
await base.Intersect_non_entity(async);

0 commit comments

Comments
 (0)