Skip to content

Commit e57a700

Browse files
rojiroji
committed
Fix DefaultIfEmpty lifting logic within SelectMany
Fixes #33343 Fixes #19095
1 parent afda113 commit e57a700

File tree

8 files changed

+223
-74
lines changed

8 files changed

+223
-74
lines changed

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

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,6 +1162,7 @@ private sealed class CorrelationFindingExpressionVisitor : ExpressionVisitor
11621162
private ParameterExpression? _outerParameter;
11631163
private bool _correlated;
11641164
private bool _defaultIfEmpty;
1165+
private bool _canLiftDefaultIfEmpty;
11651166

11661167
public (LambdaExpression, bool, bool) IsCorrelated(LambdaExpression lambdaExpression)
11671168
{
@@ -1170,6 +1171,7 @@ private sealed class CorrelationFindingExpressionVisitor : ExpressionVisitor
11701171

11711172
_correlated = false;
11721173
_defaultIfEmpty = false;
1174+
_canLiftDefaultIfEmpty = true;
11731175
_outerParameter = lambdaExpression.Parameters[0];
11741176

11751177
var result = Visit(lambdaExpression.Body);
@@ -1189,14 +1191,83 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
11891191

11901192
protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
11911193
{
1192-
if (methodCallExpression.Method.IsGenericMethod
1194+
if (_canLiftDefaultIfEmpty
1195+
&& methodCallExpression.Method.IsGenericMethod
11931196
&& methodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.DefaultIfEmptyWithoutArgument)
11941197
{
11951198
_defaultIfEmpty = true;
11961199
return Visit(methodCallExpression.Arguments[0]);
11971200
}
11981201

1199-
return base.VisitMethodCall(methodCallExpression);
1202+
if (!SupportsLiftingDefaultIfEmpty(methodCallExpression.Method))
1203+
{
1204+
// Set state to indicate that any DefaultIfEmpty encountered below this operator cannot be lifted out, since
1205+
// doing so would change meaning.
1206+
// For example, with blogs.SelectMany(b => b.Posts.DefaultIfEmpty().Select(p => p.Id)) we can lift
1207+
// the DIE out, translating the SelectMany as a LEFT JOIN (or OUTER APPLY).
1208+
// But with blogs.SelectMany(b => b.Posts.DefaultIfEmpty().Where(p => p.Id > 3)), we can't do that since that
1209+
// what result in different results.
1210+
_canLiftDefaultIfEmpty = false;
1211+
}
1212+
1213+
if (methodCallExpression.Arguments.Count == 0)
1214+
{
1215+
return base.VisitMethodCall(methodCallExpression);
1216+
}
1217+
1218+
// We need to visit the method call as usual, but the first argument - the source (other operators we're composed over) -
1219+
// needs to be handled differently. For the purpose of lifting DefaultIfEmpty, we can only do so for DIE at the top-level
1220+
// operator chain, and not some DIE embedded in e.g. the lambda argument of a Where clause. So we visit the source first,
1221+
// and then set _canLiftDefaultIfEmpty to false to avoid lifting any DIEs encountered there (see e.g. #33343).
1222+
// Note: we assume that the first argument is the source.
1223+
var newObject = Visit(methodCallExpression.Object);
1224+
1225+
var arguments = methodCallExpression.Arguments;
1226+
Expression[]? newArguments = null;
1227+
1228+
var newSource = Visit(arguments[0]);
1229+
if (!ReferenceEquals(newSource, arguments[0]))
1230+
{
1231+
newArguments = new Expression[arguments.Count];
1232+
newArguments[0] = newSource;
1233+
}
1234+
1235+
var previousCanLiftDefaultIfEmpty = _canLiftDefaultIfEmpty;
1236+
_canLiftDefaultIfEmpty = false;
1237+
1238+
for (var i = 1; i < arguments.Count; i++)
1239+
{
1240+
var newArgument = Visit(arguments[i]);
1241+
1242+
if (newArguments is not null)
1243+
{
1244+
newArguments[i] = newArgument;
1245+
}
1246+
else if (!ReferenceEquals(newArgument, arguments[i]))
1247+
{
1248+
newArguments = new Expression[arguments.Count];
1249+
newArguments[0] = newSource;
1250+
1251+
for (var j = 1; j < i; j++)
1252+
{
1253+
newArguments[j] = arguments[j];
1254+
}
1255+
1256+
newArguments[i] = newArgument;
1257+
}
1258+
}
1259+
1260+
_canLiftDefaultIfEmpty = previousCanLiftDefaultIfEmpty;
1261+
1262+
return methodCallExpression.Update(newObject, newArguments ?? (IEnumerable<Expression>)arguments);
1263+
1264+
static bool SupportsLiftingDefaultIfEmpty(MethodInfo methodInfo)
1265+
=> methodInfo.IsGenericMethod
1266+
&& methodInfo.GetGenericMethodDefinition() is var definition
1267+
&& (definition == QueryableMethods.Select
1268+
|| definition == QueryableMethods.OrderBy
1269+
|| definition == QueryableMethods.OrderByDescending
1270+
|| definition == QueryableMethods.Reverse);
12001271
}
12011272
}
12021273

‎test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1104,6 +1104,14 @@ public override async Task SelectMany_with_multiple_Take(bool async)
11041104
AssertSql();
11051105
}
11061106

1107+
public override async Task SelectMany_with_nested_DefaultIfEmpty(bool async)
1108+
{
1109+
// Cosmos client evaluation. Issue #17246.
1110+
await AssertTranslationFailed(() => base.SelectMany_with_nested_DefaultIfEmpty(async));
1111+
1112+
AssertSql();
1113+
}
1114+
11071115
public override async Task Select_with_multiple_Take(bool async)
11081116
{
11091117
// Cosmos client evaluation. Issue #17246.

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1448,6 +1448,19 @@ public virtual Task SelectMany_with_multiple_Take(bool async)
14481448
async,
14491449
ss => ss.Set<Customer>().SelectMany(c => c.Orders.OrderBy(o => o.OrderID).Take(5).Take(3)));
14501450

1451+
[ConditionalTheory] // #33343
1452+
[MemberData(nameof(IsAsyncData))]
1453+
public virtual Task SelectMany_with_nested_DefaultIfEmpty(bool async)
1454+
=> AssertQuery(
1455+
async,
1456+
ss => ss.Set<Customer>()
1457+
.SelectMany(c => c.Orders
1458+
// Make sure no orders are actually returned;
1459+
// if the DIE below is erroneously lifted as in #3343, that would cause a change in results
1460+
.Where(p => false)
1461+
.SelectMany(o => o.OrderDetails.DefaultIfEmpty())),
1462+
assertEmpty: true);
1463+
14511464
[ConditionalTheory]
14521465
[MemberData(nameof(IsAsyncData))]
14531466
public virtual Task Select_with_multiple_Take(bool async)

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3857,25 +3857,35 @@ FROM [LevelOne] AS [l]
38573857

38583858
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
38593859
{
3860-
// DefaultIfEmpty on child collection. Issue #19095.
3861-
await Assert.ThrowsAsync<EqualException>(
3862-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
3860+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
38633861

38643862
AssertSql(
38653863
"""
38663864
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
38673865
FROM [LevelOne] AS [l]
3868-
OUTER APPLY (
3866+
CROSS APPLY (
38693867
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
3870-
FROM [LevelTwo] AS [l0]
3871-
LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
3868+
FROM (
3869+
SELECT 1 AS empty
3870+
) AS [e]
3871+
LEFT JOIN (
3872+
SELECT [l0].[Id]
3873+
FROM [LevelTwo] AS [l0]
3874+
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
3875+
) AS [l1] ON 1 = 1
3876+
LEFT JOIN [LevelThree] AS [l2] ON [l1].[Id] = [l2].[Id]
38723877
CROSS APPLY (
3873-
SELECT [l].[Name] AS [l1Name], [l1].[Name] AS [l2Name], [l3].[Name] AS [l3Name]
3874-
FROM [LevelFour] AS [l2]
3875-
LEFT JOIN [LevelThree] AS [l3] ON [l2].[OneToOne_Optional_PK_Inverse4Id] = [l3].[Id]
3876-
WHERE [l1].[Id] IS NOT NULL AND [l1].[Id] = [l2].[OneToMany_Optional_Inverse4Id]
3878+
SELECT [l].[Name] AS [l1Name], [l2].[Name] AS [l2Name], [l5].[Name] AS [l3Name]
3879+
FROM (
3880+
SELECT 1 AS empty
3881+
) AS [e0]
3882+
LEFT JOIN (
3883+
SELECT [l3].[OneToOne_Optional_PK_Inverse4Id]
3884+
FROM [LevelFour] AS [l3]
3885+
WHERE [l2].[Id] IS NOT NULL AND [l2].[Id] = [l3].[OneToMany_Optional_Inverse4Id]
3886+
) AS [l4] ON 1 = 1
3887+
LEFT JOIN [LevelThree] AS [l5] ON [l4].[OneToOne_Optional_PK_Inverse4Id] = [l5].[Id]
38773888
) AS [s]
3878-
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
38793889
) AS [s0]
38803890
""");
38813891
}

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3857,25 +3857,35 @@ FROM [LevelOne] AS [l]
38573857

38583858
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
38593859
{
3860-
// DefaultIfEmpty on child collection. Issue #19095.
3861-
await Assert.ThrowsAsync<EqualException>(
3862-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
3860+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
38633861

38643862
AssertSql(
38653863
"""
38663864
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
38673865
FROM [LevelOne] AS [l]
3868-
OUTER APPLY (
3866+
CROSS APPLY (
38693867
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
3870-
FROM [LevelTwo] AS [l0]
3871-
LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
3868+
FROM (
3869+
SELECT 1 AS empty
3870+
) AS [e]
3871+
LEFT JOIN (
3872+
SELECT [l0].[Id]
3873+
FROM [LevelTwo] AS [l0]
3874+
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
3875+
) AS [l1] ON 1 = 1
3876+
LEFT JOIN [LevelThree] AS [l2] ON [l1].[Id] = [l2].[Id]
38723877
CROSS APPLY (
3873-
SELECT [l].[Name] AS [l1Name], [l1].[Name] AS [l2Name], [l3].[Name] AS [l3Name]
3874-
FROM [LevelFour] AS [l2]
3875-
LEFT JOIN [LevelThree] AS [l3] ON [l2].[OneToOne_Optional_PK_Inverse4Id] = [l3].[Id]
3876-
WHERE [l1].[Id] IS NOT NULL AND [l1].[Id] = [l2].[OneToMany_Optional_Inverse4Id]
3878+
SELECT [l].[Name] AS [l1Name], [l2].[Name] AS [l2Name], [l5].[Name] AS [l3Name]
3879+
FROM (
3880+
SELECT 1 AS empty
3881+
) AS [e0]
3882+
LEFT JOIN (
3883+
SELECT [l3].[OneToOne_Optional_PK_Inverse4Id]
3884+
FROM [LevelFour] AS [l3]
3885+
WHERE [l2].[Id] IS NOT NULL AND [l2].[Id] = [l3].[OneToMany_Optional_Inverse4Id]
3886+
) AS [l4] ON 1 = 1
3887+
LEFT JOIN [LevelThree] AS [l5] ON [l4].[OneToOne_Optional_PK_Inverse4Id] = [l5].[Id]
38773888
) AS [s]
3878-
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
38793889
) AS [s0]
38803890
""");
38813891
}

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

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -437,45 +437,55 @@ END IS NULL
437437

438438
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
439439
{
440-
// DefaultIfEmpty on child collection. Issue #19095.
441-
await Assert.ThrowsAsync<EqualException>(
442-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
440+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
443441

444442
AssertSql(
445443
"""
446444
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
447445
FROM [Level1] AS [l]
448-
OUTER APPLY (
446+
CROSS APPLY (
449447
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
450-
FROM [Level1] AS [l0]
448+
FROM (
449+
SELECT 1 AS empty
450+
) AS [e]
451451
LEFT JOIN (
452-
SELECT [l1].[Id], [l1].[Level2_Required_Id], [l1].[Level3_Name], [l1].[OneToMany_Required_Inverse3Id]
453-
FROM [Level1] AS [l1]
454-
WHERE [l1].[Level2_Required_Id] IS NOT NULL AND [l1].[OneToMany_Required_Inverse3Id] IS NOT NULL
455-
) AS [l2] ON CASE
456-
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]
452+
SELECT [l0].[Id], [l0].[OneToOne_Required_PK_Date], [l0].[Level1_Required_Id], [l0].[OneToMany_Required_Inverse2Id]
453+
FROM [Level1] AS [l0]
454+
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 AND [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
455+
) AS [l1] ON 1 = 1
456+
LEFT JOIN (
457+
SELECT [l2].[Id], [l2].[Level2_Required_Id], [l2].[Level3_Name], [l2].[OneToMany_Required_Inverse3Id]
458+
FROM [Level1] AS [l2]
459+
WHERE [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL
460+
) AS [l3] ON CASE
461+
WHEN [l1].[OneToOne_Required_PK_Date] IS NOT NULL AND [l1].[Level1_Required_Id] IS NOT NULL AND [l1].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l1].[Id]
457462
END = CASE
458-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
463+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
459464
END
460465
CROSS APPLY (
461-
SELECT [l].[Name] AS [l1Name], [l2].[Level3_Name] AS [l2Name], [l5].[Level3_Name] AS [l3Name]
462-
FROM [Level1] AS [l3]
466+
SELECT [l].[Name] AS [l1Name], [l3].[Level3_Name] AS [l2Name], [l7].[Level3_Name] AS [l3Name]
467+
FROM (
468+
SELECT 1 AS empty
469+
) AS [e0]
463470
LEFT JOIN (
464-
SELECT [l4].[Id], [l4].[Level2_Required_Id], [l4].[Level3_Name], [l4].[OneToMany_Required_Inverse3Id]
471+
SELECT [l4].[OneToOne_Optional_PK_Inverse4Id]
465472
FROM [Level1] AS [l4]
466-
WHERE [l4].[Level2_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse3Id] IS NOT NULL
467-
) AS [l5] ON [l3].[OneToOne_Optional_PK_Inverse4Id] = CASE
468-
WHEN [l5].[Level2_Required_Id] IS NOT NULL AND [l5].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l5].[Id]
473+
WHERE [l4].[Level3_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse4Id] IS NOT NULL AND CASE
474+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
475+
END IS NOT NULL AND (CASE
476+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
477+
END = [l4].[OneToMany_Optional_Inverse4Id] OR (CASE
478+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
479+
END IS NULL AND [l4].[OneToMany_Optional_Inverse4Id] IS NULL))
480+
) AS [l5] ON 1 = 1
481+
LEFT JOIN (
482+
SELECT [l6].[Id], [l6].[Level2_Required_Id], [l6].[Level3_Name], [l6].[OneToMany_Required_Inverse3Id]
483+
FROM [Level1] AS [l6]
484+
WHERE [l6].[Level2_Required_Id] IS NOT NULL AND [l6].[OneToMany_Required_Inverse3Id] IS NOT NULL
485+
) AS [l7] ON [l5].[OneToOne_Optional_PK_Inverse4Id] = CASE
486+
WHEN [l7].[Level2_Required_Id] IS NOT NULL AND [l7].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l7].[Id]
469487
END
470-
WHERE [l3].[Level3_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse4Id] IS NOT NULL AND CASE
471-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
472-
END IS NOT NULL AND (CASE
473-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
474-
END = [l3].[OneToMany_Optional_Inverse4Id] OR (CASE
475-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
476-
END IS NULL AND [l3].[OneToMany_Optional_Inverse4Id] IS NULL))
477488
) AS [s]
478-
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 AND [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
479489
) AS [s0]
480490
""");
481491
}

0 commit comments

Comments
 (0)