Skip to content

Commit

Permalink
Fix to #27072 - Suboptimal SQL generation for query with optional nav…
Browse files Browse the repository at this point in the history
…igation, its collection navigation and lateral join

When expanding collection navigation we added a check to filter out rows for which parent value is null and only afterwards match inner and outer keys. This could happen when chaining collection navigation after optional reference. Problem was that during translation we try to convert the collection subquery to a join and if that succeeds we don't need the null check anymore - joins key comparison doesn't match nulls. However, we also remove the null check when we are not able to convert to JOIN and use APPLY instead.

Fix is to save the original predicate before we try to extract the join predicate and then for APPLY case re-apply the original predicate and only use the extracted join predicate for the JOIN case.

Fixes #27072
  • Loading branch information
maumar committed Jan 28, 2022
1 parent 10b4433 commit d5003bc
Show file tree
Hide file tree
Showing 13 changed files with 183 additions and 24 deletions.
24 changes: 23 additions & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2104,6 +2104,10 @@ private void AddJoin(
innerSelectExpression.Limit = null;
innerSelectExpression.Offset = null;

var originalInnerSelectPredicate = innerSelectExpression.GroupBy.Count > 0
? innerSelectExpression.Having
: innerSelectExpression.Predicate;

joinPredicate = TryExtractJoinKey(this, innerSelectExpression, allowNonEquality: limit == null && offset == null);
if (joinPredicate != null)
{
Expand Down Expand Up @@ -2171,7 +2175,25 @@ private void AddJoin(
return;
}

innerSelectExpression.ApplyPredicate(joinPredicate);
if (!AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27072", out var enabled27072) || !enabled27072)
{
if (originalInnerSelectPredicate != null)
{
if (innerSelectExpression.GroupBy.Count > 0)
{
innerSelectExpression.Having = originalInnerSelectPredicate;
}
else
{
innerSelectExpression.Predicate = originalInnerSelectPredicate;
}
}
}
else
{
innerSelectExpression.ApplyPredicate(joinPredicate);
}

joinPredicate = null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2584,5 +2584,63 @@ public virtual Task Complex_query_issue_21665(bool async)
});
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Projecting_collection_after_optional_reference_correlated_with_parent(bool async)
=> AssertQuery(
async,
ss => ss.Set<Level1>().Select(l1 => new
{
Id = l1.Id,
Collection = l1.OneToOne_Optional_FK1.OneToMany_Optional2.Select(l3 => new { ChildId = l3.Id, ParentName = l1.OneToOne_Optional_FK1.Name })
}),
ss => ss.Set<Level1>().Select(l1 => new
{
Id = l1.Id,
Collection = l1.Maybe(x => x.OneToOne_Optional_FK1.Maybe(xx => xx.OneToMany_Optional2.Select(l3 => new { ChildId = (int)l3.MaybeScalar(xxx => xxx.Id), ParentName = l1.OneToOne_Optional_FK1.Maybe(xxx => xxx.Name) })))
}),
elementSorter: e => e.Id,
elementAsserter: (e, a) =>
{
Assert.Equal(e.Id, a.Id);

if (!(e.Collection == null && a.Collection != null && a.Collection.Count() == 0))
{
AssertCollection(e.Collection, a.Collection, elementSorter: ee => ee.ChildId);
}
});

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(bool async)
=> AssertQuery(
async,
ss => ss.Set<Level1>().Select(l1 => new
{
Id = l1.Id,
Entity = l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2,
Collection = l1.OneToOne_Optional_FK1.OneToMany_Optional2.GroupBy(x => x.Name).Select(g => new { g.Key, Count = g.Count() })
}),
ss => ss.Set<Level1>().Select(l1 => new
{
Id = l1.Id,
Entity = l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2,
Collection = l1.Maybe(x => x.OneToOne_Optional_FK1.Maybe(xx => xx.OneToMany_Optional2.GroupBy(x => x.Name).Select(g => new { g.Key, Count = g.Count() })))
}),
elementSorter: e => e.Id,
elementAsserter: (e, a) =>
{
Assert.Equal(e.Id, a.Id);
AssertEqual(e.Entity, a.Entity);
if (!(e.Collection == null && a.Collection != null && a.Collection.Count() == 0))
{
AssertCollection(e.Collection, a.Collection, elementSorter: ee => ee.Key);
}
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2242,6 +2242,40 @@ OFFSET 1 ROWS FETCH NEXT 5 ROWS ONLY
ORDER BY [t].[Name], [t].[Id], [t].[Id0], [t0].[Name], [t0].[Id]");
}

public override async Task Projecting_collection_after_optional_reference_correlated_with_parent(bool async)
{
await base.Projecting_collection_after_optional_reference_correlated_with_parent(async);

AssertSql(
@"SELECT [l].[Id], [l0].[Id], [t].[ChildId], [t].[ParentName]
FROM [LevelOne] AS [l]
LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
OUTER APPLY (
SELECT [l1].[Id] AS [ChildId], [l0].[Name] AS [ParentName]
FROM [LevelThree] AS [l1]
WHERE ([l0].[Id] IS NOT NULL) AND ([l0].[Id] = [l1].[OneToMany_Optional_Inverse3Id])
) AS [t]
ORDER BY [l].[Id], [l0].[Id]");
}

public override async Task Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(bool async)
{
await base.Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(async);

AssertSql(
@"SELECT [l].[Id], [l1].[Id], [l1].[Level2_Optional_Id], [l1].[Level2_Required_Id], [l1].[Name], [l1].[OneToMany_Optional_Inverse3Id], [l1].[OneToMany_Optional_Self_Inverse3Id], [l1].[OneToMany_Required_Inverse3Id], [l1].[OneToMany_Required_Self_Inverse3Id], [l1].[OneToOne_Optional_PK_Inverse3Id], [l1].[OneToOne_Optional_Self3Id], [l0].[Id], [t].[Key], [t].[Count]
FROM [LevelOne] AS [l]
LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Level2_Optional_Id]
OUTER APPLY (
SELECT [l2].[Name] AS [Key], COUNT(*) AS [Count]
FROM [LevelThree] AS [l2]
WHERE ([l0].[Id] IS NOT NULL) AND ([l0].[Id] = [l2].[OneToMany_Optional_Inverse3Id])
GROUP BY [l2].[Name]
) AS [t]
ORDER BY [l].[Id], [l0].[Id], [l1].[Id]");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4121,7 +4121,7 @@ FROM [Gears] AS [g]
OUTER APPLY (
SELECT [g0].[FullName] AS [ReportName], [g0].[Nickname], [g0].[SquadId]
FROM [Gears] AS [g0]
WHERE ([g].[FullName] <> N'Foo') AND (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId]))
WHERE (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId])) AND ([g].[FullName] <> N'Foo')
) AS [t]
WHERE [g].[Discriminator] = N'Officer'
ORDER BY [g].[Nickname], [g].[SquadId], [t].[Nickname]");
Expand All @@ -4140,7 +4140,7 @@ FROM [Gears] AS [g0]
OUTER APPLY (
SELECT [w].[Name], [g0].[Nickname], [w].[Id]
FROM [Weapons] AS [w]
WHERE (([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL)) AND ([g0].[FullName] = [w].[OwnerFullName])
WHERE ([g0].[FullName] = [w].[OwnerFullName]) AND (([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL))
) AS [t]
WHERE [g0].[FullName] <> N'Foo'
) AS [t0] ON ([g].[Nickname] = [t0].[LeaderNickname]) AND ([g].[SquadId] = [t0].[LeaderSquadId])
Expand All @@ -4163,7 +4163,7 @@ LEFT JOIN (
FROM [Weapons] AS [w]
WHERE ([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL)
) AS [t] ON [g0].[FullName] = [t].[OwnerFullName]
WHERE ([g0].[FullName] <> N'Foo') AND (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId]))
WHERE (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId])) AND ([g0].[FullName] <> N'Foo')
) AS [t0]
WHERE [g].[Discriminator] = N'Officer'
ORDER BY [g].[Nickname], [g].[SquadId], [t0].[Nickname], [t0].[SquadId]");
Expand Down Expand Up @@ -7939,7 +7939,7 @@ FROM [Gears] AS [g]
OUTER APPLY (
SELECT [t].[Id], [t].[Name], [t0].[Nickname], [t0].[FullName], [t0].[HasSoulPatch], [w].[Id] AS [Id0]
FROM [Weapons] AS [w]
WHERE [t0].[FullName] = [w].[OwnerFullName]
WHERE [w].[OwnerFullName] = [t0].[FullName]
) AS [t2]
) AS [t1]
ORDER BY [t].[Id], [t1].[Nickname], [t1].[FullName], [t1].[HasSoulPatch]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ FROM [Customers] AS [c]
CROSS APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
LEFT JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
Expand All @@ -555,7 +555,7 @@ FROM [Customers] AS [c]
OUTER APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
LEFT JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.EntityFrameworkCore.Query
Expand Down Expand Up @@ -1604,7 +1601,7 @@ FROM [Orders] AS [o]
OUTER APPLY (
SELECT [t].[CustomerID], [o0].[OrderID], [o0].[OrderDate]
FROM [Orders] AS [o0]
WHERE [o0].[OrderID] IN (10248, 10249, 10250) AND (([t].[CustomerID] = [o0].[CustomerID]) OR (([t].[CustomerID] IS NULL) AND ([o0].[CustomerID] IS NULL)))
WHERE (([t].[CustomerID] IS NOT NULL) AND ([t].[CustomerID] = [o0].[CustomerID])) AND [o0].[OrderID] IN (10248, 10249, 10250)
) AS [t0]
ORDER BY [t].[CustomerID], [t0].[OrderID]");
}
Expand All @@ -1622,7 +1619,7 @@ FROM [Orders] AS [o]
OUTER APPLY (
SELECT [t].[OrderID] AS [Outer], [o0].[OrderID] AS [Inner], [o0].[OrderDate]
FROM [Orders] AS [o0]
WHERE [o0].[OrderID] IN (10248, 10249, 10250) AND ([t].[OrderID] = [o0].[OrderID])
WHERE ([o0].[OrderID] = [t].[OrderID]) AND [o0].[OrderID] IN (10248, 10249, 10250)
) AS [t0]
ORDER BY [t].[OrderID]");
}
Expand All @@ -1640,7 +1637,7 @@ FROM [Orders] AS [o]
OUTER APPLY (
SELECT [t].[OrderDate] AS [Outer1], [t].[CustomerID] AS [Outer2], [o0].[OrderID] AS [Inner], [o0].[OrderDate]
FROM [Orders] AS [o0]
WHERE [o0].[OrderID] IN (10248, 10249, 10250) AND (([t].[CustomerID] = [o0].[CustomerID]) OR (([t].[CustomerID] IS NULL) AND ([o0].[CustomerID] IS NULL)))
WHERE (([o0].[CustomerID] = [t].[CustomerID]) OR (([o0].[CustomerID] IS NULL) AND ([t].[CustomerID] IS NULL))) AND [o0].[OrderID] IN (10248, 10249, 10250)
) AS [t0]
ORDER BY [t].[OrderDate], [t].[CustomerID]");
}
Expand Down Expand Up @@ -1677,7 +1674,7 @@ FROM [Orders] AS [o]
OUTER APPLY (
SELECT [t].[OrderID] AS [Outer], [o0].[OrderID] AS [Inner], [o0].[OrderDate]
FROM [Orders] AS [o0]
WHERE [o0].[OrderID] IN (10248, 10249, 10250) AND ([t].[OrderID] = [o0].[OrderID])
WHERE ([o0].[OrderID] = [t].[OrderID]) AND [o0].[OrderID] IN (10248, 10249, 10250)
) AS [t0]
ORDER BY [t].[OrderID]");
}
Expand All @@ -1697,7 +1694,7 @@ OUTER APPLY (
FROM [Orders] AS [o0]
WHERE [o].[CustomerID] = [c].[CustomerID]
) AS [t]
WHERE (DATEPART(year, [o].[OrderDate]) = 1997) AND ([c].[CustomerID] = [o].[CustomerID])
WHERE ([o].[CustomerID] = [c].[CustomerID]) AND (DATEPART(year, [o].[OrderDate]) = 1997)
) AS [t0]
WHERE [c].[City] = N'London'
ORDER BY [c].[CustomerID], [t0].[OrderID], [t0].[OrderID00]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,7 @@ FROM [Customers] AS [c]
CROSS APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
WHERE [c].[CustomerID] LIKE N'F%'
Expand All @@ -697,7 +697,7 @@ FROM [Customers] AS [c]
CROSS APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
INNER JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
Expand All @@ -715,7 +715,7 @@ FROM [Customers] AS [c]
OUTER APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
WHERE [c].[CustomerID] LIKE N'F%'
Expand All @@ -726,7 +726,7 @@ FROM [Customers] AS [c]
OUTER APPLY (
SELECT TOP(5) [o].[OrderID]
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID]
WHERE [o].[CustomerID] = [c].[CustomerID]
ORDER BY [c].[CustomerID]
) AS [t]
INNER JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5607,10 +5607,10 @@ OUTER APPLY (
SELECT [a1].[Id], [a1].[ActivityTypeId], [a1].[CompetitionSeasonId], [a1].[Points], [c0].[Id] AS [Id0]
FROM [ActivityTypePoints12456] AS [a1]
INNER JOIN [CompetitionSeasons] AS [c0] ON [a1].[CompetitionSeasonId] = [c0].[Id]
WHERE ([c0].[Id] = (
WHERE ([a0].[Id] = [a1].[ActivityTypeId]) AND ([c0].[Id] = (
SELECT TOP(1) [c1].[Id]
FROM [CompetitionSeasons] AS [c1]
WHERE ([c1].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c1].[EndDate]))) AND ([a0].[Id] = [a1].[ActivityTypeId])
WHERE ([c1].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c1].[EndDate])))
) AS [t]
ORDER BY [a].[Id], [a0].[Id], [t].[Id]");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4920,7 +4920,7 @@ LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[Squad
OUTER APPLY (
SELECT [g0].[FullName] AS [ReportName], [g0].[Nickname], [g0].[SquadId]
FROM [Gears] AS [g0]
WHERE ([g].[FullName] <> N'Foo') AND (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId]))
WHERE (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId])) AND ([g].[FullName] <> N'Foo')
) AS [t]
WHERE [o].[Nickname] IS NOT NULL
ORDER BY [g].[Nickname], [g].[SquadId], [t].[Nickname]");
Expand All @@ -4940,7 +4940,7 @@ FROM [Gears] AS [g0]
OUTER APPLY (
SELECT [w].[Name], [g0].[Nickname], [w].[Id]
FROM [Weapons] AS [w]
WHERE (([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL)) AND ([g0].[FullName] = [w].[OwnerFullName])
WHERE ([g0].[FullName] = [w].[OwnerFullName]) AND (([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL))
) AS [t]
WHERE [g0].[FullName] <> N'Foo'
) AS [t0] ON ([g].[Nickname] = [t0].[LeaderNickname]) AND ([g].[SquadId] = [t0].[LeaderSquadId])
Expand All @@ -4964,7 +4964,7 @@ LEFT JOIN (
FROM [Weapons] AS [w]
WHERE ([w].[Name] <> N'Bar') OR ([w].[Name] IS NULL)
) AS [t] ON [g0].[FullName] = [t].[OwnerFullName]
WHERE ([g0].[FullName] <> N'Foo') AND (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId]))
WHERE (([g].[Nickname] = [g0].[LeaderNickname]) AND ([g].[SquadId] = [g0].[LeaderSquadId])) AND ([g0].[FullName] <> N'Foo')
) AS [t0]
WHERE [o].[Nickname] IS NOT NULL
ORDER BY [g].[Nickname], [g].[SquadId], [t0].[Nickname], [t0].[SquadId]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,5 +141,17 @@ public override async Task Complex_query_issue_21665(bool async)
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Complex_query_issue_21665(async))).Message);

public override async Task Projecting_collection_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_after_optional_reference_correlated_with_parent(async))).Message);

public override async Task Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(async))).Message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,5 +125,17 @@ public override async Task SelectMany_with_predicate_and_DefaultIfEmpty_projecti
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.SelectMany_with_predicate_and_DefaultIfEmpty_projecting_root_collection_element_and_another_collection(async))).Message);

public override async Task Projecting_collection_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_after_optional_reference_correlated_with_parent(async))).Message);

public override async Task Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(async))).Message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,5 +117,17 @@ public override async Task Complex_query_issue_21665(bool async)
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Complex_query_issue_21665(async))).Message);

public override async Task Projecting_collection_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_after_optional_reference_correlated_with_parent(async))).Message);

public override async Task Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Projecting_collection_with_group_by_after_optional_reference_correlated_with_parent(async))).Message);
}
}
Loading

0 comments on commit d5003bc

Please sign in to comment.