Skip to content

Work on inlined primitive collections (VALUES) #36159

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,7 @@ JsonScalarExpression jsonScalar
var sqlExpression = sqlExpressions[i];
rowExpressions[i] =
new RowValueExpression(
new[]
{
[
// Since VALUES may not guarantee row ordering, we add an _ord value by which we'll order.
_sqlExpressionFactory.Constant(i, intTypeMapping),
// If no type mapping was inferred (i.e. no column in the inline collection), it's left null, to allow it to get
Expand All @@ -412,11 +411,11 @@ JsonScalarExpression jsonScalar
sqlExpression.TypeMapping is null && inferredTypeMaping is not null
? _sqlExpressionFactory.ApplyTypeMapping(sqlExpression, inferredTypeMaping)
: sqlExpression
});
]);
}

var alias = _sqlAliasManager.GenerateTableAlias("values");
var valuesExpression = new ValuesExpression(alias, rowExpressions, new[] { ValuesOrderingColumnName, ValuesValueColumnName });
var valuesExpression = new ValuesExpression(alias, rowExpressions, [ValuesOrderingColumnName, ValuesValueColumnName]);

return CreateShapedQueryExpressionForValuesExpression(
valuesExpression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,7 @@ when TryGetInferredTypeMapping(columnExpression.TableAlias, columnExpression.Nam
// By default, the ValuesExpression also contains an ordering by a synthetic increasing _ord. If the containing
// SelectExpression doesn't project it out or require it (limit/offset), strip that out.
// TODO: Strictly-speaking, stripping the ordering doesn't belong in this visitor which is about applying type mappings
return ApplyTypeMappingsOnValuesExpression(
valuesExpression,
stripOrdering: _currentSelectExpression is { Limit: null, Offset: null }
&& !_currentSelectExpression.Projection.Any(
p => p.Expression is ColumnExpression
{
Name: RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName
} c
&& c.TableAlias == valuesExpression.Alias));
return ApplyTypeMappingsOnValuesExpression(valuesExpression);

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

switch (valuesExpression)
{
Expand All @@ -159,14 +147,9 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
for (var i = 0; i < newRowValues.Length; i++)
{
var rowValue = rowValues[i];
var newValues = new SqlExpression[newColumnNames.Count];
var newValues = new SqlExpression[valuesExpression.ColumnNames.Count];
for (var j = 0; j < valuesExpression.ColumnNames.Count; j++)
{
if (j == 0 && stripOrdering)
{
continue;
}

var value = rowValue.Values[j];

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

newValues[j - (stripOrdering ? 1 : 0)] = value;
newValues[j] = value;
}

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

return new ValuesExpression(valuesExpression.Alias, newRowValues, null, newColumnNames);
return valuesExpression.Update(newRowValues);
}

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

return new ValuesExpression(
valuesExpression.Alias,
(SqlParameterExpression)valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping),
newColumnNames);
return valuesExpression.Update(valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping));
}

default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ public SqlParameterExpression(
/// </summary>
/// <param name="typeMapping">A relational type mapping to apply.</param>
/// <returns>A new expression which has supplied type mapping.</returns>
public SqlExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping)
=> new SqlParameterExpression(InvariantName, Name, Type, IsNullable, ShouldBeConstantized, typeMapping);
public SqlParameterExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping)
=> new(InvariantName, Name, Type, IsNullable, ShouldBeConstantized, typeMapping);

/// <inheritdoc />
protected override Expression VisitChildren(ExpressionVisitor visitor)
Expand Down
37 changes: 28 additions & 9 deletions src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,29 +113,48 @@ protected override Expression VisitExtension(Expression node)
return join.Update(newTable, newJoinPredicate);
}

case ValuesExpression { ValuesParameter: SqlParameterExpression valuesParameter } valuesExpression:
case ValuesExpression { ValuesParameter: SqlParameterExpression valuesParameter, ColumnNames: var columnNames } valuesExpression:
{
DoNotCache();
Check.DebugAssert(valuesParameter.TypeMapping is not null, "valuesParameter.TypeMapping is not null");
Check.DebugAssert(
valuesParameter.TypeMapping.ElementTypeMapping is not null,
"valuesParameter.TypeMapping.ElementTypeMapping is not null");

// If the ordering column hasn't been pruned, we need to create it as well
Check.DebugAssert(
columnNames.Count == 1
|| columnNames.Count == 2 && columnNames[0] == RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName,
"Unhandled type of ValuesExpression");
var addOrdering = columnNames[0] == RelationalQueryableMethodTranslatingExpressionVisitor.ValuesOrderingColumnName;

var typeMapping = (RelationalTypeMapping)valuesParameter.TypeMapping.ElementTypeMapping;
var values = (IEnumerable?)ParameterValues[valuesParameter.Name] ?? Array.Empty<object>();

var processedValues = new List<RowValueExpression>();

var intTypeMapping = Dependencies.TypeMappingSource.FindMapping(typeof(int))!;
var i = 0;

foreach (var value in values)
{
var valueExpression = _sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), sensitive: true, typeMapping);

// We currently add explicit conversions on the first row (but not to the _ord column), to ensure that the inferred
// types are properly typed. See #30605 for removing that when not needed.
if (i == 0 && value is not ColumnExpression)
{
valueExpression = new SqlUnaryExpression(ExpressionType.Convert, valueExpression, valueExpression.Type, typeMapping);
}

processedValues.Add(
new RowValueExpression(
[
_sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), sensitive: true, typeMapping)
]));
new RowValueExpression(addOrdering
? [_sqlExpressionFactory.Constant(i, intTypeMapping), valueExpression]
: [valueExpression]));

i++;
}

return processedValues is not []
? valuesExpression.Update(processedValues)
: valuesExpression;
return valuesExpression.Update(processedValues);
}

default:
Expand Down
98 changes: 98 additions & 0 deletions src/EFCore.Relational/Query/SqlTreePruner.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;

namespace Microsoft.EntityFrameworkCore.Query;
Expand Down Expand Up @@ -117,6 +118,9 @@ protected override Expression VisitExtension(Expression node)
PruneSelect(source2, preserveProjection: true));
}

case ValuesExpression values:
return PruneValues(values);

default:
return base.VisitExtension(node);
}
Expand Down Expand Up @@ -262,4 +266,98 @@ protected virtual SelectExpression PruneSelect(SelectExpression select, bool pre
return select.Update(
tables ?? select.Tables, predicate, groupBy, having, projections ?? select.Projection, orderings, offset, limit);
}

/// <summary>
/// Prunes a <see cref="ValuesExpression" />, removing columns inside it which aren't referenced.
/// This currently removes the <c>_ord</c> column that gets added to preserve ordering, for cases where
/// that ordering isn't actually necessary.
/// </summary>
/// <remarks>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </remarks>
[EntityFrameworkInternal]
protected virtual ValuesExpression PruneValues(ValuesExpression values)
{
if (!ReferencedColumnMap.TryGetValue(values.Alias, out var referencedColumnNames))
{
throw new UnreachableException("The case of a totally unreferenced ValuesExpression is already handled for all tables in PruneSelect.");
}

// First, build a bitmap of which columns are referenced which we can efficiently access later
// as we traverse the rows. At the same time, build a list of the names of the referenced columns.
BitArray? referencedColumns = null;
List<string>? newColumnNames = null;
for (var i = 0; i < values.ColumnNames.Count; i++)
{
var columnName = values.ColumnNames[i];
var isColumnReferenced = referencedColumnNames.Contains(columnName);

if (newColumnNames is null && !isColumnReferenced)
{
newColumnNames = new List<string>(values.ColumnNames.Count);
referencedColumns = new BitArray(values.ColumnNames.Count);

for (var j = 0; j < i; j++)
{
referencedColumns[j] = true;
newColumnNames.Add(columnName);
}
}

if (newColumnNames is not null)
{
if (isColumnReferenced)
{
newColumnNames.Add(columnName);
}

referencedColumns![i] = isColumnReferenced;
}
}

if (referencedColumns is null)
{
return values;
}

// We know at least some columns are getting pruned.
Debug.Assert(newColumnNames is not null);

switch (values)
{
// If we have a value parameter (row values aren't specific in line), we still prune the column names.
// Later in SqlNullabilityProcessor, when the parameterized collection is inline to constants, we'll take
// the column names into account.
case ValuesExpression { ValuesParameter: not null }:
return new ValuesExpression(values.Alias, rowValues: null, values.ValuesParameter, newColumnNames);

// Go over the rows and create new ones without the pruned columns.
case ValuesExpression { RowValues: IReadOnlyList<RowValueExpression> rowValues }:
var newRowValues = new RowValueExpression[rowValues.Count];

for (var i = 0; i < rowValues.Count; i++)
{
var oldValues = rowValues[i].Values;
var newValues = new List<SqlExpression>(newColumnNames.Count);

for (var j = 0; j < values.ColumnNames.Count; j++)
{
if (referencedColumns[j])
{
newValues.Add(oldValues[j]);
}
}

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

return new ValuesExpression(values.Alias, newRowValues, valuesParameter: null, newColumnNames);

default:
throw new UnreachableException();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,17 @@ FROM root c
}
}

public override async Task Inline_collection_index_Column_with_EF_Constant(bool async)
{
// Always throws for sync.
if (async)
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => base.Inline_collection_index_Column_with_EF_Constant(async));

Assert.Equal(CoreStrings.EFConstantNotSupported, exception.Message);
}
}

public override async Task Inline_collection_value_index_Column(bool async)
{
// Always throws for sync.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,18 @@ public virtual Task Inline_collection_index_Column(bool async)
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => new[] { 1, 2, 3 }[c.Int] == 1),
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? new[] { 1, 2, 3 }[c.Int] : -1) == 1));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Inline_collection_index_Column_with_EF_Constant(bool async)
{
int[] ints = [1, 2, 3];

return AssertQuery(
async,
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => EF.Constant(ints)[c.Int] == 1),
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => (c.Int <= 2 ? ints[c.Int] : -1) == 1));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Inline_collection_value_index_Column(bool async)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2431,7 +2431,7 @@ WHERE [t].[Id] IN (?, ?, ?)
FROM [TestEntities] AS [t]
WHERE EXISTS (
SELECT 1
FROM (VALUES (?), (?), (?)) AS [i]([Value])
FROM (VALUES (CAST(? AS int)), (?), (?)) AS [i]([Value])
WHERE [i].[Value] = [t].[Id])
""",
//
Expand All @@ -2455,7 +2455,7 @@ WHERE [t].[Id] IN (1, 2, 3)
FROM [TestEntities] AS [t]
WHERE EXISTS (
SELECT 1
FROM (VALUES (1), (2), (3)) AS [i]([Value])
FROM (VALUES (CAST(1 AS int)), (2), (3)) AS [i]([Value])
WHERE [i].[Value] = [t].[Id])
""",
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ SELECT [t].[Id]
FROM [TestEntity] AS [t]
WHERE (
SELECT COUNT(*)
FROM (VALUES (2), (999)) AS [i]([Value])
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
WHERE [i].[Value] > [t].[Id]) = 1
""");
}
Expand Down Expand Up @@ -897,7 +897,7 @@ SELECT [t].[Id]
FROM [TestEntity] AS [t]
WHERE (
SELECT COUNT(*)
FROM (VALUES (2), (999)) AS [i]([Value])
FROM (VALUES (CAST(2 AS int)), (999)) AS [i]([Value])
WHERE [i].[Value] > [t].[Id]) = 1
""");
}
Expand Down
Loading