Skip to content

Commit 28e66aa

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 7a337e9 commit 28e66aa

16 files changed

+259
-59
lines changed

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -401,8 +401,7 @@ JsonScalarExpression jsonScalar
401401
var sqlExpression = sqlExpressions[i];
402402
rowExpressions[i] =
403403
new RowValueExpression(
404-
new[]
405-
{
404+
[
406405
// Since VALUES may not guarantee row ordering, we add an _ord value by which we'll order.
407406
_sqlExpressionFactory.Constant(i, intTypeMapping),
408407
// If no type mapping was inferred (i.e. no column in the inline collection), it's left null, to allow it to get
@@ -412,11 +411,11 @@ JsonScalarExpression jsonScalar
412411
sqlExpression.TypeMapping is null && inferredTypeMaping is not null
413412
? _sqlExpressionFactory.ApplyTypeMapping(sqlExpression, inferredTypeMaping)
414413
: sqlExpression
415-
});
414+
]);
416415
}
417416

418417
var alias = _sqlAliasManager.GenerateTableAlias("values");
419-
var valuesExpression = new ValuesExpression(alias, rowExpressions, new[] { ValuesOrderingColumnName, ValuesValueColumnName });
418+
var valuesExpression = new ValuesExpression(alias, rowExpressions, [ValuesOrderingColumnName, ValuesValueColumnName]);
420419

421420
return CreateShapedQueryExpressionForValuesExpression(
422421
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/SqlNullabilityProcessor.cs

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -113,29 +113,48 @@ protected override Expression VisitExtension(Expression node)
113113
return join.Update(newTable, newJoinPredicate);
114114
}
115115

116-
case ValuesExpression { ValuesParameter: SqlParameterExpression valuesParameter } valuesExpression:
116+
case ValuesExpression { ValuesParameter: SqlParameterExpression valuesParameter, ColumnNames: var columnNames } valuesExpression:
117117
{
118118
DoNotCache();
119119
Check.DebugAssert(valuesParameter.TypeMapping is not null, "valuesParameter.TypeMapping is not null");
120120
Check.DebugAssert(
121121
valuesParameter.TypeMapping.ElementTypeMapping is not null,
122122
"valuesParameter.TypeMapping.ElementTypeMapping is not null");
123+
124+
// If the ordering column hasn't been pruned, we need to create it as well
125+
Check.DebugAssert(
126+
columnNames.Count == 1
127+
|| columnNames.Count == 2 && columnNames[0] == RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName,
128+
"Unhandled type of ValuesExpression");
129+
var addOrdering = columnNames[0] == RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName;
130+
123131
var typeMapping = (RelationalTypeMapping)valuesParameter.TypeMapping.ElementTypeMapping;
124132
var values = (IEnumerable?)ParameterValues[valuesParameter.Name] ?? Array.Empty<object>();
125-
126133
var processedValues = new List<RowValueExpression>();
134+
135+
var intTypeMapping = Dependencies.TypeMappingSource.FindMapping(typeof(int))!;
136+
var i = 0;
137+
127138
foreach (var value in values)
128139
{
140+
var valueExpression = _sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), sensitive: true, typeMapping);
141+
142+
// We currently add explicit conversions on the first row (but not to the _ord column), to ensure that the inferred
143+
// types are properly typed. See #30605 for removing that when not needed.
144+
if (i == 0 && value is not ColumnExpression)
145+
{
146+
valueExpression = new SqlUnaryExpression(ExpressionType.Convert, valueExpression, valueExpression.Type, typeMapping);
147+
}
148+
129149
processedValues.Add(
130-
new RowValueExpression(
131-
[
132-
_sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), sensitive: true, typeMapping)
133-
]));
150+
new RowValueExpression(addOrdering
151+
? [_sqlExpressionFactory.Constant(i, intTypeMapping), valueExpression]
152+
: [valueExpression]));
153+
154+
i++;
134155
}
135156

136-
return processedValues is not []
137-
? valuesExpression.Update(processedValues)
138-
: valuesExpression;
157+
return valuesExpression.Update(processedValues);
139158
}
140159

141160
default:

‎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
@@ -1162,6 +1162,17 @@ FROM root c
11621162
}
11631163
}
11641164

1165+
public override async Task Inline_collection_index_Column_with_EF_Constant(bool async)
1166+
{
1167+
// Always throws for sync.
1168+
if (async)
1169+
{
1170+
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => base.Inline_collection_index_Column_with_EF_Constant(async));
1171+
1172+
Assert.Equal(CoreStrings.EFConstantNotSupported, exception.Message);
1173+
}
1174+
}
1175+
11651176
public override async Task Inline_collection_value_index_Column(bool async)
11661177
{
11671178
// 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
@@ -822,6 +822,18 @@ public virtual Task Inline_collection_index_Column(bool async)
822822
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => new[] { 1, 2, 3 }[c.Int] == 1),
823823
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? new[] { 1, 2, 3 }[c.Int] : -1) == 1));
824824

825+
[ConditionalTheory]
826+
[MemberData(nameof(IsAsyncData))]
827+
public virtual Task Inline_collection_index_Column_with_EF_Constant(bool async)
828+
{
829+
int[] ints = [1, 2, 3];
830+
831+
return AssertQuery(
832+
async,
833+
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => EF.Constant(ints)[c.Int] == 1),
834+
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? ints[c.Int] : -1) == 1));
835+
}
836+
825837
[ConditionalTheory]
826838
[MemberData(nameof(IsAsyncData))]
827839
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
@@ -2431,7 +2431,7 @@ WHERE [t].[Id] IN (?, ?, ?)
24312431
FROM [TestEntities] AS [t]
24322432
WHERE EXISTS (
24332433
SELECT 1
2434-
FROM (VALUES (?), (?), (?)) AS [i]([Value])
2434+
FROM (VALUES (CAST(? AS int)), (?), (?)) AS [i]([Value])
24352435
WHERE [i].[Value] = [t].[Id])
24362436
""",
24372437
//
@@ -2455,7 +2455,7 @@ WHERE [t].[Id] IN (1, 2, 3)
24552455
FROM [TestEntities] AS [t]
24562456
WHERE EXISTS (
24572457
SELECT 1
2458-
FROM (VALUES (1), (2), (3)) AS [i]([Value])
2458+
FROM (VALUES (CAST(1 AS int)), (2), (3)) AS [i]([Value])
24592459
WHERE [i].[Value] = [t].[Id])
24602460
""",
24612461
//

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,7 @@ SELECT [t].[Id]
802802
FROM [TestEntity] AS [t]
803803
WHERE (
804804
SELECT COUNT(*)
805-
FROM (VALUES (2), (999)) AS [i]([Value])
805+
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
806806
WHERE [i].[Value] > [t].[Id]) = 1
807807
""");
808808
}
@@ -897,7 +897,7 @@ SELECT [t].[Id]
897897
FROM [TestEntity] AS [t]
898898
WHERE (
899899
SELECT COUNT(*)
900-
FROM (VALUES (2), (999)) AS [i]([Value])
900+
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
901901
WHERE [i].[Value] > [t].[Id]) = 1
902902
""");
903903
}

0 commit comments

Comments
 (0)