Skip to content

Commit dbc1fab

Browse files
maumarmaumar
committed
Fix to #15873 - Query: Identifying columns in the case of distinct
Added validation step for AddCollectionJoin which checks that if subquery contains Distinct or GroupBy, the projection contains all identifying columns needed to correctly bucket the results during materialization. Fixes #15873
1 parent f7c3ce4 commit dbc1fab

File tree

9 files changed

+154
-2
lines changed

9 files changed

+154
-2
lines changed

‎src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

+6
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/EFCore.Relational/Properties/RelationalStrings.resx

+3
Original file line numberDiff line numberDiff line change
@@ -735,4 +735,7 @@
735735
<data name="MissingOrderingInSqlExpression" xml:space="preserve">
736736
<value>Reverse could not be translated to the server because there is no ordering on the server side.</value>
737737
</data>
738+
<data name="MissingIdentifyingProjectionInDistinctGroupBySubquery" xml:space="preserve">
739+
<value>Collection subquery that uses 'Distinct' or 'Group By' operations must project key columns of all of it's tables. Missing column: {column}. Either add column(s) to the projection or rewrite query to not use 'GroupBy'/'Distinct' operation.</value>
740+
</data>
738741
</root>

‎src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

+17-2
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
using JetBrains.Annotations;
1111
using Microsoft.EntityFrameworkCore.ChangeTracking;
1212
using Microsoft.EntityFrameworkCore.Diagnostics;
13+
using Microsoft.EntityFrameworkCore.Infrastructure;
1314
using Microsoft.EntityFrameworkCore.Metadata;
1415
using Microsoft.EntityFrameworkCore.Metadata.Internal;
1516
using Microsoft.EntityFrameworkCore.Utilities;
@@ -1066,7 +1067,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
10661067

10671068
var identifiers = _identifier.ToList();
10681069
_identifier.Clear();
1069-
// TODO: See issue#15873
1070+
10701071
foreach (var identifier in identifiers)
10711072
{
10721073
if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
@@ -1083,7 +1084,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
10831084

10841085
var childIdentifiers = _childIdentifiers.ToList();
10851086
_childIdentifiers.Clear();
1086-
// TODO: See issue#15873
1087+
10871088
foreach (var identifier in childIdentifiers)
10881089
{
10891090
if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
@@ -1427,6 +1428,20 @@ public Expression ApplyCollectionJoin(
14271428
var innerClientEval = innerSelectExpression.Projection.Count > 0;
14281429
innerSelectExpression.ApplyProjection();
14291430

1431+
if (innerSelectExpression.IsDistinct
1432+
|| innerSelectExpression.GroupBy.Count > 0)
1433+
{
1434+
var innerSelectProjectionExpressions = innerSelectExpression._projection.Select(p => p.Expression).ToList();
1435+
foreach (var innerSelectIdentifier in innerSelectExpression._identifier)
1436+
{
1437+
if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column))
1438+
{
1439+
throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery(
1440+
innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name));
1441+
}
1442+
}
1443+
}
1444+
14301445
if (collectionIndex == 0)
14311446
{
14321447
foreach (var identifier in parentIdentifierList)

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

+29
Original file line numberDiff line numberDiff line change
@@ -7764,6 +7764,35 @@ from w in ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName).OrderBy(x =
77647764
});
77657765
}
77667766

7767+
[ConditionalTheory]
7768+
[MemberData(nameof(IsAsyncData))]
7769+
public virtual Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
7770+
{
7771+
return AssertQuery(
7772+
async,
7773+
ss => ss.Set<Gear>()
7774+
.OrderBy(g => g.Nickname)
7775+
.Select(g => g.Weapons.SelectMany(x => x.Owner.AssignedCity.BornGears)
7776+
.Select(x => (bool?)x.HasSoulPatch).Distinct().ToList()));
7777+
}
7778+
7779+
[ConditionalTheory]
7780+
[MemberData(nameof(IsAsyncData))]
7781+
public virtual Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
7782+
{
7783+
return AssertQuery(
7784+
async,
7785+
ss => ss.Set<Mission>()
7786+
.Select(m => new
7787+
{
7788+
m.Id,
7789+
grouping = m.ParticipatingSquads
7790+
.Select(ps => ps.SquadId)
7791+
.GroupBy(s => s)
7792+
.Select(g => new { g.Key, Count = g.Count() })
7793+
}));
7794+
}
7795+
77677796
protected GearsOfWarContext CreateContext() => Fixture.CreateContext();
77687797

77697798
protected virtual void ClearLog()

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

+18
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Linq;
56
using System.Threading.Tasks;
7+
using Microsoft.EntityFrameworkCore.Diagnostics;
68
using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
79
using Xunit;
810
using Xunit.Abstractions;
@@ -7136,6 +7138,22 @@ ORDER BY [w].[Id]
71367138
ORDER BY [g].[Nickname], [t].[Id]");
71377139
}
71387140

7141+
public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
7142+
{
7143+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
7144+
() => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
7145+
7146+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
7147+
}
7148+
7149+
public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
7150+
{
7151+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
7152+
() => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
7153+
7154+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
7155+
}
7156+
71397157
private void AssertSql(params string[] expected)
71407158
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
71417159
}

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

+27
Original file line numberDiff line numberDiff line change
@@ -5260,6 +5260,33 @@ FROM [Orders] AS [o]
52605260
FROM [Customers] AS [c]");
52615261
}
52625262

5263+
public override async Task SelectMany_correlated_subquery_hard(bool async)
5264+
{
5265+
await base.SelectMany_correlated_subquery_hard(async);
5266+
5267+
AssertSql(
5268+
@"@__p_0='91'
5269+
5270+
SELECT [t0].[City] AS [c1], [t1].[City], [t1].[City0] AS [c1]
5271+
FROM (
5272+
SELECT DISTINCT [t].[City]
5273+
FROM (
5274+
SELECT TOP(@__p_0) [c].[City], [c].[CustomerID]
5275+
FROM [Customers] AS [c]
5276+
) AS [t]
5277+
) AS [t0]
5278+
CROSS APPLY (
5279+
SELECT TOP(9) [e].[City], [t0].[City] AS [City0], [e].[EmployeeID]
5280+
FROM [Employees] AS [e]
5281+
WHERE ([t0].[City] = [e].[City]) OR ([t0].[City] IS NULL AND [e].[City] IS NULL)
5282+
) AS [t1]
5283+
CROSS APPLY (
5284+
SELECT TOP(9) [t0].[City], [e0].[EmployeeID]
5285+
FROM [Employees] AS [e0]
5286+
WHERE ([t1].[City] = [e0].[City]) OR ([t1].[City] IS NULL AND [e0].[City] IS NULL)
5287+
) AS [t2]");
5288+
}
5289+
52635290
private void AssertSql(params string[] expected)
52645291
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
52655292

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

+18
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Linq;
56
using System.Threading.Tasks;
7+
using Microsoft.EntityFrameworkCore.Diagnostics;
68
using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
79
using Xunit;
810
using Xunit.Abstractions;
@@ -8569,6 +8571,22 @@ LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[Squad
85698571
ORDER BY [c].[Location]");
85708572
}
85718573

8574+
public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
8575+
{
8576+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
8577+
() => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
8578+
8579+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
8580+
}
8581+
8582+
public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
8583+
{
8584+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
8585+
() => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
8586+
8587+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
8588+
}
8589+
85728590
private void AssertSql(params string[] expected)
85738591
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
85748592
}

‎test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs

+18
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Threading.Tasks;
6+
using Microsoft.EntityFrameworkCore.Diagnostics;
57
using Xunit;
68
using Xunit.Abstractions;
79

@@ -207,6 +209,22 @@ public override async Task Byte_array_filter_by_SequenceEqual(bool async)
207209
[ConditionalTheory(Skip = "Issue#18844")]
208210
public override Task Where_TimeSpan_Milliseconds(bool async) => base.Where_TimeSpan_Milliseconds(async);
209211

212+
public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
213+
{
214+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
215+
() => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
216+
217+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
218+
}
219+
220+
public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
221+
{
222+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
223+
() => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
224+
225+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
226+
}
227+
210228
private void AssertSql(params string[] expected)
211229
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
212230
}

‎test/EFCore.Sqlite.FunctionalTests/Query/TPTGearsOfWarQuerySqliteTest.cs

+18
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Threading.Tasks;
6+
using Microsoft.EntityFrameworkCore.Diagnostics;
57
using Xunit;
68
using Xunit.Abstractions;
79

@@ -208,6 +210,22 @@ public override async Task Byte_array_filter_by_SequenceEqual(bool async)
208210
[ConditionalTheory(Skip = "Issue#18844")]
209211
public override Task Where_TimeSpan_Milliseconds(bool async) => base.Where_TimeSpan_Milliseconds(async);
210212

213+
public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
214+
{
215+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
216+
() => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
217+
218+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
219+
}
220+
221+
public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
222+
{
223+
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
224+
() => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
225+
226+
Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
227+
}
228+
211229
private void AssertSql(params string[] expected)
212230
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
213231
}

0 commit comments

Comments
 (0)