Skip to content

Commit c94f987

Browse files
rojiroji
committed
Work on inlined primitive collections (VALUES)
* Refactor and generalize the pruning of the VALUES _ord column, moving it from RelationalTypeMappingPostprocessor to SqlPruner. * Ensure that inlined parameterized collections (EF.Constant) preserved ordering via _ord and specifies the type mapping. Fixes #36158
1 parent 8250db9 commit c94f987

15 files changed

+231
-50
lines changed

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -405,8 +405,7 @@ JsonScalarExpression jsonScalar
405405
var sqlExpression = sqlExpressions[i];
406406
rowExpressions[i] =
407407
new RowValueExpression(
408-
new[]
409-
{
408+
[
410409
// Since VALUES may not guarantee row ordering, we add an _ord value by which we'll order.
411410
_sqlExpressionFactory.Constant(i, intTypeMapping),
412411
// If no type mapping was inferred (i.e. no column in the inline collection), it's left null, to allow it to get
@@ -416,11 +415,11 @@ JsonScalarExpression jsonScalar
416415
sqlExpression.TypeMapping is null && inferredTypeMaping is not null
417416
? _sqlExpressionFactory.ApplyTypeMapping(sqlExpression, inferredTypeMaping)
418417
: sqlExpression
419-
});
418+
]);
420419
}
421420

422421
var alias = _sqlAliasManager.GenerateTableAlias("values");
423-
var valuesExpression = new ValuesExpression(alias, rowExpressions, new[] { ValuesOrderingColumnName, ValuesValueColumnName });
422+
var valuesExpression = new ValuesExpression(alias, rowExpressions, [ValuesOrderingColumnName, ValuesValueColumnName]);
424423

425424
return CreateShapedQueryExpressionForValuesExpression(
426425
valuesExpression,

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

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -107,15 +107,7 @@ when TryGetInferredTypeMapping(columnExpression.TableAlias, columnExpression.Nam
107107
// By default, the ValuesExpression also contains an ordering by a synthetic increasing _ord. If the containing
108108
// SelectExpression doesn't project it out or require it (limit/offset), strip that out.
109109
// TODO: Strictly-speaking, stripping the ordering doesn't belong in this visitor which is about applying type mappings
110-
return ApplyTypeMappingsOnValuesExpression(
111-
valuesExpression,
112-
stripOrdering: _currentSelectExpression is { Limit: null, Offset: null }
113-
&& !_currentSelectExpression.Projection.Any(
114-
p => p.Expression is ColumnExpression
115-
{
116-
Name: RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName
117-
} c
118-
&& c.TableAlias == valuesExpression.Alias));
110+
return ApplyTypeMappingsOnValuesExpression(valuesExpression);
119111

120112
// SqlExpressions without an inferred type mapping indicates a problem in EF - everything should have been inferred.
121113
// One exception is SqlFragmentExpression, which never has a type mapping.
@@ -135,8 +127,7 @@ when TryGetInferredTypeMapping(columnExpression.TableAlias, columnExpression.Nam
135127
/// As an optimization, it can also strip the first _ord column if it's determined that it isn't needed (most cases).
136128
/// </summary>
137129
/// <param name="valuesExpression">The <see cref="ValuesExpression" /> to apply the mappings to.</param>
138-
/// <param name="stripOrdering">Whether to strip the <c>_ord</c> column.</param>
139-
protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExpression valuesExpression, bool stripOrdering)
130+
protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExpression valuesExpression)
140131
{
141132
var inferredTypeMappings = TryGetInferredTypeMapping(
142133
valuesExpression.Alias, RelationalQueryableMethodTranslatingExpressionVisitor.ValuesValueColumnName, out var typeMapping)
@@ -146,9 +137,6 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
146137
Check.DebugAssert(
147138
valuesExpression.ColumnNames[0] == RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName,
148139
"First ValuesExpression column isn't the ordering column");
149-
var newColumnNames = stripOrdering
150-
? valuesExpression.ColumnNames.Skip(1).ToArray()
151-
: valuesExpression.ColumnNames;
152140

153141
switch (valuesExpression)
154142
{
@@ -159,14 +147,9 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
159147
for (var i = 0; i < newRowValues.Length; i++)
160148
{
161149
var rowValue = rowValues[i];
162-
var newValues = new SqlExpression[newColumnNames.Count];
150+
var newValues = new SqlExpression[valuesExpression.ColumnNames.Count];
163151
for (var j = 0; j < valuesExpression.ColumnNames.Count; j++)
164152
{
165-
if (j == 0 && stripOrdering)
166-
{
167-
continue;
168-
}
169-
170153
var value = rowValue.Values[j];
171154

172155
if (value.TypeMapping is null
@@ -182,13 +165,13 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
182165
value = new SqlUnaryExpression(ExpressionType.Convert, value, value.Type, value.TypeMapping);
183166
}
184167

185-
newValues[j - (stripOrdering ? 1 : 0)] = value;
168+
newValues[j] = value;
186169
}
187170

188171
newRowValues[i] = new RowValueExpression(newValues);
189172
}
190173

191-
return new ValuesExpression(valuesExpression.Alias, newRowValues, null, newColumnNames);
174+
return valuesExpression.Update(newRowValues);
192175
}
193176

194177
// VALUES over a values parameter (i.e. a parameter representing the entire collection, that will be constantized into the SQL
@@ -203,10 +186,7 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
203186
throw new UnreachableException("A RelationalTypeMapping collection type mapping could not be found");
204187
}
205188

206-
return new ValuesExpression(
207-
valuesExpression.Alias,
208-
(SqlParameterExpression)valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping),
209-
newColumnNames);
189+
return valuesExpression.Update(valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping));
210190
}
211191

212192
default:

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ public SqlParameterExpression(
7474
/// </summary>
7575
/// <param name="typeMapping">A relational type mapping to apply.</param>
7676
/// <returns>A new expression which has supplied type mapping.</returns>
77-
public SqlExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping)
78-
=> new SqlParameterExpression(InvariantName, Name, Type, IsNullable, ShouldBeConstantized, typeMapping);
77+
public SqlParameterExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping)
78+
=> new(InvariantName, Name, Type, IsNullable, ShouldBeConstantized, typeMapping);
7979

8080
/// <inheritdoc />
8181
protected override Expression VisitChildren(ExpressionVisitor visitor)

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Collections;
45
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
56

67
namespace Microsoft.EntityFrameworkCore.Query;
@@ -117,6 +118,9 @@ protected override Expression VisitExtension(Expression node)
117118
PruneSelect(source2, preserveProjection: true));
118119
}
119120

121+
case ValuesExpression values:
122+
return PruneValues(values);
123+
120124
default:
121125
return base.VisitExtension(node);
122126
}
@@ -262,4 +266,98 @@ protected virtual SelectExpression PruneSelect(SelectExpression select, bool pre
262266
return select.Update(
263267
tables ?? select.Tables, predicate, groupBy, having, projections ?? select.Projection, orderings, offset, limit);
264268
}
269+
270+
/// <summary>
271+
/// Prunes a <see cref="ValuesExpression" />, removing columns inside it which aren't referenced.
272+
/// This currently removes the <c>_ord</c> column that gets added to preserve ordering, for cases where
273+
/// that ordering isn't actually necessary.
274+
/// </summary>
275+
/// <remarks>
276+
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
277+
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
278+
/// any release. You should only use it directly in your code with extreme caution and knowing that
279+
/// doing so can result in application failures when updating to a new Entity Framework Core release.
280+
/// </remarks>
281+
[EntityFrameworkInternal]
282+
protected virtual ValuesExpression PruneValues(ValuesExpression values)
283+
{
284+
if (!ReferencedColumnMap.TryGetValue(values.Alias, out var referencedColumnNames))
285+
{
286+
throw new UnreachableException("The case of a totally unreferenced ValuesExpression is already handled for all tables in PruneSelect.");
287+
}
288+
289+
// First, build a bitmap of which columns are referenced which we can efficiently access later
290+
// as we traverse the rows. At the same time, build a list of the names of the referenced columns.
291+
BitArray? referencedColumns = null;
292+
List<string>? newColumnNames = null;
293+
for (var i = 0; i < values.ColumnNames.Count; i++)
294+
{
295+
var columnName = values.ColumnNames[i];
296+
var isColumnReferenced = referencedColumnNames.Contains(columnName);
297+
298+
if (newColumnNames is null && !isColumnReferenced)
299+
{
300+
newColumnNames = new List<string>(values.ColumnNames.Count);
301+
referencedColumns = new BitArray(values.ColumnNames.Count);
302+
303+
for (var j = 0; j < i; j++)
304+
{
305+
referencedColumns[j] = true;
306+
newColumnNames.Add(columnName);
307+
}
308+
}
309+
310+
if (newColumnNames is not null)
311+
{
312+
if (isColumnReferenced)
313+
{
314+
newColumnNames.Add(columnName);
315+
}
316+
317+
referencedColumns![i] = isColumnReferenced;
318+
}
319+
}
320+
321+
if (referencedColumns is null)
322+
{
323+
return values;
324+
}
325+
326+
// We know at least some columns are getting pruned.
327+
Debug.Assert(newColumnNames is not null);
328+
329+
switch (values)
330+
{
331+
// If we have a value parameter (row values aren't specific in line), we still prune the column names.
332+
// Later in SqlNullabilityProcessor, when the parameterized collection is inline to constants, we'll take
333+
// the column names into account.
334+
case ValuesExpression { ValuesParameter: not null }:
335+
return new ValuesExpression(values.Alias, rowValues: null, values.ValuesParameter, newColumnNames);
336+
337+
// Go over the rows and create new ones without the pruned columns.
338+
case ValuesExpression { RowValues: IReadOnlyList<RowValueExpression> rowValues }:
339+
var newRowValues = new RowValueExpression[rowValues.Count];
340+
341+
for (var i = 0; i < rowValues.Count; i++)
342+
{
343+
var oldValues = rowValues[i].Values;
344+
var newValues = new List<SqlExpression>(newColumnNames.Count);
345+
346+
for (var j = 0; j < values.ColumnNames.Count; j++)
347+
{
348+
if (referencedColumns[j])
349+
{
350+
newValues.Add(oldValues[j]);
351+
}
352+
}
353+
354+
newRowValues[i] = new RowValueExpression(newValues);
355+
}
356+
357+
return new ValuesExpression(values.Alias, newRowValues, valuesParameter: null, newColumnNames);
358+
359+
default:
360+
throw new UnreachableException();
361+
}
362+
}
265363
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,6 +1158,17 @@ FROM root c
11581158
}
11591159
}
11601160

1161+
public override async Task Inline_collection_index_Column_with_EF_Constant(bool async)
1162+
{
1163+
// Always throws for sync.
1164+
if (async)
1165+
{
1166+
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => base.Inline_collection_index_Column_with_EF_Constant(async));
1167+
1168+
Assert.Equal(CoreStrings.EFConstantNotSupported, exception.Message);
1169+
}
1170+
}
1171+
11611172
public override async Task Inline_collection_value_index_Column(bool async)
11621173
{
11631174
// Always throws for sync.

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -849,6 +849,18 @@ public virtual Task Inline_collection_index_Column(bool async)
849849
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => new[] { 1, 2, 3 }[c.Int] == 1),
850850
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? new[] { 1, 2, 3 }[c.Int] : -1) == 1));
851851

852+
[ConditionalTheory]
853+
[MemberData(nameof(IsAsyncData))]
854+
public virtual Task Inline_collection_index_Column_with_EF_Constant(bool async)
855+
{
856+
int[] ints = [1, 2, 3];
857+
858+
return AssertQuery(
859+
async,
860+
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => EF.Constant(ints)[c.Int] == 1),
861+
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? ints[c.Int] : -1) == 1));
862+
}
863+
852864
[ConditionalTheory]
853865
[MemberData(nameof(IsAsyncData))]
854866
public virtual Task Inline_collection_value_index_Column(bool async)

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,7 +2505,7 @@ WHERE [t].[Id] IN (?, ?, ?)
25052505
FROM [TestEntities] AS [t]
25062506
WHERE EXISTS (
25072507
SELECT 1
2508-
FROM (VALUES (?), (?), (?)) AS [i]([Value])
2508+
FROM (VALUES (CAST(? AS int)), (?), (?)) AS [i]([Value])
25092509
WHERE [i].[Value] = [t].[Id])
25102510
""",
25112511
//
@@ -2529,7 +2529,7 @@ WHERE [t].[Id] IN (1, 2, 3)
25292529
FROM [TestEntities] AS [t]
25302530
WHERE EXISTS (
25312531
SELECT 1
2532-
FROM (VALUES (1), (2), (3)) AS [i]([Value])
2532+
FROM (VALUES (CAST(1 AS int)), (2), (3)) AS [i]([Value])
25332533
WHERE [i].[Value] = [t].[Id])
25342534
""",
25352535
//

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -795,7 +795,7 @@ SELECT [t].[Id]
795795
FROM [TestEntity] AS [t]
796796
WHERE (
797797
SELECT COUNT(*)
798-
FROM (VALUES (2), (999)) AS [i]([Value])
798+
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
799799
WHERE [i].[Value] > [t].[Id]) = 1
800800
""");
801801
}
@@ -890,7 +890,7 @@ SELECT [t].[Id]
890890
FROM [TestEntity] AS [t]
891891
WHERE (
892892
SELECT COUNT(*)
893-
FROM (VALUES (2), (999)) AS [i]([Value])
893+
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
894894
WHERE [i].[Value] > [t].[Id]) = 1
895895
""");
896896
}

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -791,7 +791,7 @@ public override async Task Parameter_collection_Where_with_EF_Constant_Where_Any
791791
FROM [PrimitiveCollectionsEntity] AS [p]
792792
WHERE EXISTS (
793793
SELECT 1
794-
FROM (VALUES (2), (999), (1000)) AS [i]([Value])
794+
FROM (VALUES (CAST(2 AS int)), (999), (1000)) AS [i]([Value])
795795
WHERE [i].[Value] > 0)
796796
""");
797797
}
@@ -806,7 +806,7 @@ public override async Task Parameter_collection_Count_with_column_predicate_with
806806
FROM [PrimitiveCollectionsEntity] AS [p]
807807
WHERE (
808808
SELECT COUNT(*)
809-
FROM (VALUES (2), (999), (1000)) AS [i]([Value])
809+
FROM (VALUES (CAST(2 AS int)), (999), (1000)) AS [i]([Value])
810810
WHERE [i].[Value] > [p].[Id]) = 2
811811
""");
812812
}
@@ -901,6 +901,22 @@ ORDER BY [v].[_ord]
901901
""");
902902
}
903903

904+
public override async Task Inline_collection_index_Column_with_EF_Constant(bool async)
905+
{
906+
await base.Inline_collection_index_Column_with_EF_Constant(async);
907+
908+
AssertSql(
909+
"""
910+
SELECT [p].[Id], [p].[Bool], [p].[Bools], [p].[DateTime], [p].[DateTimes], [p].[Enum], [p].[Enums], [p].[Int], [p].[Ints], [p].[NullableInt], [p].[NullableInts], [p].[NullableString], [p].[NullableStrings], [p].[NullableWrappedId], [p].[NullableWrappedIdWithNullableComparer], [p].[String], [p].[Strings], [p].[WrappedId]
911+
FROM [PrimitiveCollectionsEntity] AS [p]
912+
WHERE (
913+
SELECT [i].[Value]
914+
FROM (VALUES (0, CAST(1 AS int)), (1, 2), (2, 3)) AS [i]([_ord], [Value])
915+
ORDER BY [i].[_ord]
916+
OFFSET [p].[Int] ROWS FETCH NEXT 1 ROWS ONLY) = 1
917+
""");
918+
}
919+
904920
public override async Task Inline_collection_value_index_Column(bool async)
905921
{
906922
await base.Inline_collection_value_index_Column(async);

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,7 @@ public override async Task Parameter_collection_Where_with_EF_Constant_Where_Any
784784
FROM [PrimitiveCollectionsEntity] AS [p]
785785
WHERE EXISTS (
786786
SELECT 1
787-
FROM (VALUES (2), (999), (1000)) AS [i]([Value])
787+
FROM (VALUES (CAST(2 AS int)), (999), (1000)) AS [i]([Value])
788788
WHERE [i].[Value] > 0)
789789
""");
790790
}
@@ -799,7 +799,7 @@ public override async Task Parameter_collection_Count_with_column_predicate_with
799799
FROM [PrimitiveCollectionsEntity] AS [p]
800800
WHERE (
801801
SELECT COUNT(*)
802-
FROM (VALUES (2), (999), (1000)) AS [i]([Value])
802+
FROM (VALUES (CAST(2 AS int)), (999), (1000)) AS [i]([Value])
803803
WHERE [i].[Value] > [p].[Id]) = 2
804804
""");
805805
}
@@ -1066,6 +1066,22 @@ ORDER BY [v].[_ord]
10661066
""");
10671067
}
10681068

1069+
public override async Task Inline_collection_index_Column_with_EF_Constant(bool async)
1070+
{
1071+
await base.Inline_collection_index_Column_with_EF_Constant(async);
1072+
1073+
AssertSql(
1074+
"""
1075+
SELECT [p].[Id], [p].[Bool], [p].[Bools], [p].[DateTime], [p].[DateTimes], [p].[Enum], [p].[Enums], [p].[Int], [p].[Ints], [p].[NullableInt], [p].[NullableInts], [p].[NullableString], [p].[NullableStrings], [p].[NullableWrappedId], [p].[NullableWrappedIdWithNullableComparer], [p].[String], [p].[Strings], [p].[WrappedId]
1076+
FROM [PrimitiveCollectionsEntity] AS [p]
1077+
WHERE (
1078+
SELECT [i].[Value]
1079+
FROM (VALUES (0, CAST(1 AS int)), (1, 2), (2, 3)) AS [i]([_ord], [Value])
1080+
ORDER BY [i].[_ord]
1081+
OFFSET [p].[Int] ROWS FETCH NEXT 1 ROWS ONLY) = 1
1082+
""");
1083+
}
1084+
10691085
public override async Task Inline_collection_value_index_Column(bool async)
10701086
{
10711087
await base.Inline_collection_value_index_Column(async);

0 commit comments

Comments
 (0)