Skip to content

Commit 2c27fcd

Browse files
rojiroji
authored
Fix DefaultIfEmpty lifting logic within SelectMany (#36248)
Fixes #33343 Fixes #19095
1 parent cba8dcf commit 2c27fcd

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
@@ -1113,6 +1113,14 @@ public override async Task SelectMany_with_multiple_Take(bool async)
11131113
AssertSql();
11141114
}
11151115

1116+
public override async Task SelectMany_with_nested_DefaultIfEmpty(bool async)
1117+
{
1118+
// Cosmos client evaluation. Issue #17246.
1119+
await AssertTranslationFailed(() => base.SelectMany_with_nested_DefaultIfEmpty(async));
1120+
1121+
AssertSql();
1122+
}
1123+
11161124
public override async Task Select_with_multiple_Take(bool async)
11171125
{
11181126
// 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
@@ -1456,6 +1456,19 @@ public virtual Task SelectMany_with_multiple_Take(bool async)
14561456
async,
14571457
ss => ss.Set<Customer>().SelectMany(c => c.Orders.OrderBy(o => o.OrderID).Take(5).Take(3)));
14581458

1459+
[ConditionalTheory] // #33343
1460+
[MemberData(nameof(IsAsyncData))]
1461+
public virtual Task SelectMany_with_nested_DefaultIfEmpty(bool async)
1462+
=> AssertQuery(
1463+
async,
1464+
ss => ss.Set<Customer>()
1465+
.SelectMany(c => c.Orders
1466+
// Make sure no orders are actually returned;
1467+
// if the DIE below is erroneously lifted as in #3343, that would cause a change in results
1468+
.Where(p => false)
1469+
.SelectMany(o => o.OrderDetails.DefaultIfEmpty())),
1470+
assertEmpty: true);
1471+
14591472
[ConditionalTheory]
14601473
[MemberData(nameof(IsAsyncData))]
14611474
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
@@ -3853,25 +3853,35 @@ FROM [LevelOne] AS [l]
38533853

38543854
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
38553855
{
3856-
// DefaultIfEmpty on child collection. Issue #19095.
3857-
await Assert.ThrowsAsync<EqualException>(
3858-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
3856+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
38593857

38603858
AssertSql(
38613859
"""
38623860
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
38633861
FROM [LevelOne] AS [l]
3864-
OUTER APPLY (
3862+
CROSS APPLY (
38653863
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
3866-
FROM [LevelTwo] AS [l0]
3867-
LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
3864+
FROM (
3865+
SELECT 1 AS empty
3866+
) AS [e]
3867+
LEFT JOIN (
3868+
SELECT [l0].[Id]
3869+
FROM [LevelTwo] AS [l0]
3870+
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
3871+
) AS [l1] ON 1 = 1
3872+
LEFT JOIN [LevelThree] AS [l2] ON [l1].[Id] = [l2].[Id]
38683873
CROSS APPLY (
3869-
SELECT [l].[Name] AS [l1Name], [l1].[Name] AS [l2Name], [l3].[Name] AS [l3Name]
3870-
FROM [LevelFour] AS [l2]
3871-
LEFT JOIN [LevelThree] AS [l3] ON [l2].[OneToOne_Optional_PK_Inverse4Id] = [l3].[Id]
3872-
WHERE [l1].[Id] IS NOT NULL AND [l1].[Id] = [l2].[OneToMany_Optional_Inverse4Id]
3874+
SELECT [l].[Name] AS [l1Name], [l2].[Name] AS [l2Name], [l5].[Name] AS [l3Name]
3875+
FROM (
3876+
SELECT 1 AS empty
3877+
) AS [e0]
3878+
LEFT JOIN (
3879+
SELECT [l3].[OneToOne_Optional_PK_Inverse4Id]
3880+
FROM [LevelFour] AS [l3]
3881+
WHERE [l2].[Id] IS NOT NULL AND [l2].[Id] = [l3].[OneToMany_Optional_Inverse4Id]
3882+
) AS [l4] ON 1 = 1
3883+
LEFT JOIN [LevelThree] AS [l5] ON [l4].[OneToOne_Optional_PK_Inverse4Id] = [l5].[Id]
38733884
) AS [s]
3874-
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
38753885
) AS [s0]
38763886
""");
38773887
}

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

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

38543854
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
38553855
{
3856-
// DefaultIfEmpty on child collection. Issue #19095.
3857-
await Assert.ThrowsAsync<EqualException>(
3858-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
3856+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
38593857

38603858
AssertSql(
38613859
"""
38623860
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
38633861
FROM [LevelOne] AS [l]
3864-
OUTER APPLY (
3862+
CROSS APPLY (
38653863
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
3866-
FROM [LevelTwo] AS [l0]
3867-
LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
3864+
FROM (
3865+
SELECT 1 AS empty
3866+
) AS [e]
3867+
LEFT JOIN (
3868+
SELECT [l0].[Id]
3869+
FROM [LevelTwo] AS [l0]
3870+
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
3871+
) AS [l1] ON 1 = 1
3872+
LEFT JOIN [LevelThree] AS [l2] ON [l1].[Id] = [l2].[Id]
38683873
CROSS APPLY (
3869-
SELECT [l].[Name] AS [l1Name], [l1].[Name] AS [l2Name], [l3].[Name] AS [l3Name]
3870-
FROM [LevelFour] AS [l2]
3871-
LEFT JOIN [LevelThree] AS [l3] ON [l2].[OneToOne_Optional_PK_Inverse4Id] = [l3].[Id]
3872-
WHERE [l1].[Id] IS NOT NULL AND [l1].[Id] = [l2].[OneToMany_Optional_Inverse4Id]
3874+
SELECT [l].[Name] AS [l1Name], [l2].[Name] AS [l2Name], [l5].[Name] AS [l3Name]
3875+
FROM (
3876+
SELECT 1 AS empty
3877+
) AS [e0]
3878+
LEFT JOIN (
3879+
SELECT [l3].[OneToOne_Optional_PK_Inverse4Id]
3880+
FROM [LevelFour] AS [l3]
3881+
WHERE [l2].[Id] IS NOT NULL AND [l2].[Id] = [l3].[OneToMany_Optional_Inverse4Id]
3882+
) AS [l4] ON 1 = 1
3883+
LEFT JOIN [LevelThree] AS [l5] ON [l4].[OneToOne_Optional_PK_Inverse4Id] = [l5].[Id]
38733884
) AS [s]
3874-
WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
38753885
) AS [s0]
38763886
""");
38773887
}

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

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

432432
public override async Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
433433
{
434-
// DefaultIfEmpty on child collection. Issue #19095.
435-
await Assert.ThrowsAsync<EqualException>(
436-
async () => await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async));
434+
await base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
437435

438436
AssertSql(
439437
"""
440438
SELECT [s0].[l1Name], [s0].[l2Name], [s0].[l3Name]
441439
FROM [Level1] AS [l]
442-
OUTER APPLY (
440+
CROSS APPLY (
443441
SELECT [s].[l1Name], [s].[l2Name], [s].[l3Name]
444-
FROM [Level1] AS [l0]
442+
FROM (
443+
SELECT 1 AS empty
444+
) AS [e]
445445
LEFT JOIN (
446-
SELECT [l1].[Id], [l1].[Level2_Required_Id], [l1].[Level3_Name], [l1].[OneToMany_Required_Inverse3Id]
447-
FROM [Level1] AS [l1]
448-
WHERE [l1].[Level2_Required_Id] IS NOT NULL AND [l1].[OneToMany_Required_Inverse3Id] IS NOT NULL
449-
) AS [l2] ON CASE
450-
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]
446+
SELECT [l0].[Id], [l0].[OneToOne_Required_PK_Date], [l0].[Level1_Required_Id], [l0].[OneToMany_Required_Inverse2Id]
447+
FROM [Level1] AS [l0]
448+
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]
449+
) AS [l1] ON 1 = 1
450+
LEFT JOIN (
451+
SELECT [l2].[Id], [l2].[Level2_Required_Id], [l2].[Level3_Name], [l2].[OneToMany_Required_Inverse3Id]
452+
FROM [Level1] AS [l2]
453+
WHERE [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL
454+
) AS [l3] ON CASE
455+
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]
451456
END = CASE
452-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
457+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
453458
END
454459
CROSS APPLY (
455-
SELECT [l].[Name] AS [l1Name], [l2].[Level3_Name] AS [l2Name], [l5].[Level3_Name] AS [l3Name]
456-
FROM [Level1] AS [l3]
460+
SELECT [l].[Name] AS [l1Name], [l3].[Level3_Name] AS [l2Name], [l7].[Level3_Name] AS [l3Name]
461+
FROM (
462+
SELECT 1 AS empty
463+
) AS [e0]
457464
LEFT JOIN (
458-
SELECT [l4].[Id], [l4].[Level2_Required_Id], [l4].[Level3_Name], [l4].[OneToMany_Required_Inverse3Id]
465+
SELECT [l4].[OneToOne_Optional_PK_Inverse4Id]
459466
FROM [Level1] AS [l4]
460-
WHERE [l4].[Level2_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse3Id] IS NOT NULL
461-
) AS [l5] ON [l3].[OneToOne_Optional_PK_Inverse4Id] = CASE
462-
WHEN [l5].[Level2_Required_Id] IS NOT NULL AND [l5].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l5].[Id]
467+
WHERE [l4].[Level3_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse4Id] IS NOT NULL AND CASE
468+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
469+
END IS NOT NULL AND (CASE
470+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
471+
END = [l4].[OneToMany_Optional_Inverse4Id] OR (CASE
472+
WHEN [l3].[Level2_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l3].[Id]
473+
END IS NULL AND [l4].[OneToMany_Optional_Inverse4Id] IS NULL))
474+
) AS [l5] ON 1 = 1
475+
LEFT JOIN (
476+
SELECT [l6].[Id], [l6].[Level2_Required_Id], [l6].[Level3_Name], [l6].[OneToMany_Required_Inverse3Id]
477+
FROM [Level1] AS [l6]
478+
WHERE [l6].[Level2_Required_Id] IS NOT NULL AND [l6].[OneToMany_Required_Inverse3Id] IS NOT NULL
479+
) AS [l7] ON [l5].[OneToOne_Optional_PK_Inverse4Id] = CASE
480+
WHEN [l7].[Level2_Required_Id] IS NOT NULL AND [l7].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l7].[Id]
463481
END
464-
WHERE [l3].[Level3_Required_Id] IS NOT NULL AND [l3].[OneToMany_Required_Inverse4Id] IS NOT NULL AND CASE
465-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
466-
END IS NOT NULL AND (CASE
467-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
468-
END = [l3].[OneToMany_Optional_Inverse4Id] OR (CASE
469-
WHEN [l2].[Level2_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL THEN [l2].[Id]
470-
END IS NULL AND [l3].[OneToMany_Optional_Inverse4Id] IS NULL))
471482
) AS [s]
472-
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]
473483
) AS [s0]
474484
""");
475485
}

0 commit comments

Comments
 (0)