Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0] Fix to #27072 - Suboptimal SQL generation for query with optional navigation, its collection navigation and lateral join #27302

Merged
merged 1 commit into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
maumar marked this conversation as resolved.
Show resolved Hide resolved
) 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