Skip to content

Commit

Permalink
WIP
Browse files Browse the repository at this point in the history 
Closes dotnet#30732
Closes dotnet#30734
  • Loading branch information
@roji
roji committed Apr 27, 2023
1 parent 2edfca8 commit db7ec78
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 94 deletions.
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/RelationalQueryRootProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public RelationalQueryRootProcessor(
/// Indicates that a <see cref="ConstantExpression" /> can be converted to a <see cref="InlineQueryRootExpression" />;
/// this will later be translated to a SQL <see cref="ValuesExpression" />.
/// </summary>
protected override bool ShouldConvertToInlineQueryRoot(ConstantExpression constantExpression)
protected override bool ShouldConvertToInlineQueryRoot(NewArrayExpression newArrayExpression)
=> true;

/// <inheritdoc />
Expand Down
54 changes: 24 additions & 30 deletions src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Reflection.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
Expand Down Expand Up @@ -207,8 +208,8 @@ when entityQueryRootExpression.GetType() == typeof(EntityQueryRootExpression)
return new ShapedQueryExpression(selectExpression, shaperExpression);
}

case InlineQueryRootExpression constantQueryRootExpression:
return VisitInlineQueryRoot(constantQueryRootExpression) ?? base.VisitExtension(extensionExpression);
case InlineQueryRootExpression inlineQueryRootExpression:
return VisitInlineQueryRoot(inlineQueryRootExpression) ?? base.VisitExtension(extensionExpression);

case ParameterQueryRootExpression parameterQueryRootExpression:
var sqlParameterExpression =
Expand Down Expand Up @@ -319,26 +320,24 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp

for (var i = 0; i < inlineQueryRootExpression.Values.Count; i++)
{
var value = inlineQueryRootExpression.Values[i];

// We currently support constants only; supporting non-constant values in VALUES is tracked by #30734.
if (value is not ConstantExpression constantExpression)
// TODO: Don't apply default type mapping
if (TranslateExpression(inlineQueryRootExpression.Values[i]) is not SqlExpression translatedValue)
{
AddTranslationErrorDetails(RelationalStrings.OnlyConstantsSupportedInInlineCollectionQueryRoots);
return null;
}

if (constantExpression.Value is null)
{
encounteredNull = true;
}
// TODO: Poor man's null semantics - the ValuesExpression projects out a non-nullable column if we see only non-null constants
// or nullable columns.
// This should be handled properly, possibly in SqlNullabilityProcessor (e.g. any complex expression is assumed to be nullable).
encounteredNull |=
translatedValue is not SqlConstantExpression { Value: not null } and not ColumnExpression { IsNullable: false };

rowExpressions.Add(new RowValueExpression(new[]
{
// Since VALUES may not guarantee row ordering, we add an _ord value by which we'll order.
_sqlExpressionFactory.Constant(i, intTypeMapping),
// Note that for the actual value, we must leave the type mapping null to allow it to get inferred later based on usage
_sqlExpressionFactory.Constant(constantExpression.Value, elementType, typeMapping: null)
translatedValue
}));
}

Expand Down Expand Up @@ -1802,7 +1801,7 @@ protected virtual bool IsValidSelectExpressionForExecuteUpdate(
protected virtual Expression ApplyInferredTypeMappings(
Expression expression,
IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping> inferredTypeMappings)
=> new RelationalInferredTypeMappingApplier(inferredTypeMappings).Visit(expression);
=> new RelationalInferredTypeMappingApplier(_sqlExpressionFactory, inferredTypeMappings).Visit(expression);

/// <summary>
/// Determines whether the given <see cref="SelectExpression" /> is ordered, typically because orderings have been added to it.
Expand Down Expand Up @@ -1874,15 +1873,7 @@ private bool TrySimplifyValuesToInExpression(
for (var i = 0; i < values.Length; i++)
{
// Skip the first value (_ord), which is irrelevant for Contains
if (valuesExpression.RowValues[i].Values[1] is SqlConstantExpression { Value: var constantValue })
{
values[i] = constantValue;
}
else
{
simplifiedQuery = null;
return false;
}
values[i] = valuesExpression.RowValues[i].Values[1];
}

var inExpression = _sqlExpressionFactory.In(item, _sqlExpressionFactory.Constant(values), isNegated);
Expand Down Expand Up @@ -2751,6 +2742,7 @@ private void RegisterInferredTypeMapping(ColumnExpression columnExpression, Rela
/// </summary>
protected class RelationalInferredTypeMappingApplier : ExpressionVisitor
{
private readonly ISqlExpressionFactory _sqlExpressionFactory;
private SelectExpression? _currentSelectExpression;

/// <summary>
Expand All @@ -2761,10 +2753,15 @@ protected class RelationalInferredTypeMappingApplier : ExpressionVisitor
/// <summary>
/// Creates a new instance of the <see cref="RelationalInferredTypeMappingApplier" /> class.
/// </summary>
/// <param name="sqlExpressionFactory">The SQL expression factory.</param>
/// <param name="inferredTypeMappings">The inferred type mappings to be applied back on their query roots.</param>
public RelationalInferredTypeMappingApplier(
ISqlExpressionFactory sqlExpressionFactory,
IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping> inferredTypeMappings)
=> InferredTypeMappings = inferredTypeMappings;
{
_sqlExpressionFactory = sqlExpressionFactory;
InferredTypeMappings = inferredTypeMappings;
}

/// <inheritdoc />
protected override Expression VisitExtension(Expression expression)
Expand Down Expand Up @@ -2831,30 +2828,27 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
var newValues = new SqlExpression[newColumnNames.Count];
for (var j = 0; j < valuesExpression.ColumnNames.Count; j++)
{
Check.DebugAssert(rowValue.Values[j] is SqlConstantExpression, "Non-constant SqlExpression in ValuesExpression");

if (j == 0 && stripOrdering)
{
continue;
}

var value = (SqlConstantExpression)rowValue.Values[j];
SqlExpression newValue = value;
var value = rowValue.Values[j];

var inferredTypeMapping = inferredTypeMappings[j];
if (inferredTypeMapping is not null && value.TypeMapping is null)
{
newValue = new SqlConstantExpression(Expression.Constant(value.Value, value.Type), inferredTypeMapping);
value = _sqlExpressionFactory.ApplyTypeMapping(value, inferredTypeMapping);

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

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

newRowValues[i] = new RowValueExpression(newValues);
Expand Down
20 changes: 3 additions & 17 deletions src/EFCore.Relational/Query/SqlExpressions/ValuesExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,9 @@ public ValuesExpression(
{
Check.NotEmpty(rowValues, nameof(rowValues));

#if DEBUG
if (rowValues.Any(rv => rv.Values.Count != columnNames.Count))
{
throw new ArgumentException("All number of all row values doesn't match the number of column names");
}

if (rowValues.SelectMany(rv => rv.Values).Any(
v => v is not SqlConstantExpression and not SqlUnaryExpression
{
Operand: SqlConstantExpression,
OperatorType: ExpressionType.Convert
}))
{
// See #30734 for non-constants
throw new ArgumentException("Only constant expressions are supported in ValuesExpression");
}
#endif
Check.DebugAssert(
rowValues.All(rv => rv.Values.Count == columnNames.Count),
"All row values must have a value count matching the number of column names");

RowValues = rowValues;
ColumnNames = columnNames;
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ public SqlServerInferredTypeMappingApplier(
IRelationalTypeMappingSource typeMappingSource,
ISqlExpressionFactory sqlExpressionFactory,
IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping> inferredTypeMappings)
: base(inferredTypeMappings)
: base(sqlExpressionFactory, inferredTypeMappings)
=> (_typeMappingSource, _sqlExpressionFactory) = (typeMappingSource, sqlExpressionFactory);

/// <summary>
Expand Down
5 changes: 2 additions & 3 deletions src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,9 +644,8 @@ protected override Expression VisitConstant(ConstantExpression constantExpressio
}

private static bool IsEvaluatableNodeType(Expression expression)
=> expression.NodeType != ExpressionType.Extension
|| expression.CanReduce
&& IsEvaluatableNodeType(expression.ReduceAndCheck());
=> expression.NodeType is not ExpressionType.Extension and not ExpressionType.NewArrayInit
|| expression.CanReduce && IsEvaluatableNodeType(expression.ReduceAndCheck());

private static bool IsQueryableMethod(Expression expression)
=> expression is MethodCallExpression methodCallExpression
Expand Down
77 changes: 41 additions & 36 deletions src/EFCore/Query/QueryRootProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,48 +50,18 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
var argument = methodCallExpression.Arguments[i];
var parameterType = parameters[i].ParameterType;

Expression? visitedArgument = null;

// This converts collections over constants and parameters to query roots, for later translation of LINQ operators over them.
// The element type doesn't have to be directly mappable; we allow unknown CLR types in order to support value convertors
// (the precise type mapping - with the value converter - will be inferred later based on LINQ operators composed on the root).
// However, we do exclude element CLR types which are associated to entity types in our model, since Contains over entity
// collections isn't yet supported (#30712).
if (parameterType.IsGenericType
var visitedArgument = parameterType.IsGenericType
&& (parameterType.GetGenericTypeDefinition() == typeof(IEnumerable<>)
|| parameterType.GetGenericTypeDefinition() == typeof(IQueryable<>))
&& parameterType.GetGenericArguments()[0] is var elementClrType
&& !_model.FindEntityTypes(elementClrType).Any())
{
switch (argument)
{
case ConstantExpression { Value: IEnumerable values } constantExpression
when ShouldConvertToInlineQueryRoot(constantExpression):

var valueExpressions = new List<ConstantExpression>();
foreach (var value in values)
{
valueExpressions.Add(Expression.Constant(value, elementClrType));
}
visitedArgument = new InlineQueryRootExpression(valueExpressions, elementClrType);
break;

// TODO: Support NewArrayExpression, see #30734.

case ParameterExpression parameterExpression
when parameterExpression.Name?.StartsWith(QueryCompilationContext.QueryParameterPrefix, StringComparison.Ordinal)
== true
&& ShouldConvertToParameterQueryRoot(parameterExpression):
visitedArgument = new ParameterQueryRootExpression(parameterExpression.Type.GetSequenceType(), parameterExpression);
break;

default:
visitedArgument = null;
break;
}
}

visitedArgument ??= Visit(argument);
&& !_model.FindEntityTypes(elementClrType).Any()
? VisitQueryRootCandidate(argument, elementClrType)
: Visit(argument);

if (visitedArgument != argument)
{
Expand All @@ -117,12 +87,47 @@ when ShouldConvertToInlineQueryRoot(constantExpression):
: methodCallExpression.Update(methodCallExpression.Object, newArguments);
}

private Expression VisitQueryRootCandidate(Expression expression, Type elementClrType)
{
switch (expression)
{
// An array containing only constants is represented as a ConstantExpression with the array as the value.
// Convert that into a NewArrayExpression for use with InlineQueryRootExpression
case ConstantExpression { Value: IEnumerable values }:
var valueExpressions = new List<ConstantExpression>();
foreach (var value in values)
{
valueExpressions.Add(Expression.Constant(value, elementClrType));
}

if (ShouldConvertToInlineQueryRoot(Expression.NewArrayInit(elementClrType, valueExpressions)))
{
return new InlineQueryRootExpression(valueExpressions, elementClrType);
}

goto default;

case NewArrayExpression newArrayExpression
when ShouldConvertToInlineQueryRoot(newArrayExpression):
return new InlineQueryRootExpression(newArrayExpression.Expressions, elementClrType);

case ParameterExpression parameterExpression
when parameterExpression.Name?.StartsWith(QueryCompilationContext.QueryParameterPrefix, StringComparison.Ordinal)
== true
&& ShouldConvertToParameterQueryRoot(parameterExpression):
return new ParameterQueryRootExpression(parameterExpression.Type.GetSequenceType(), parameterExpression);

default:
return Visit(expression);
}
}

/// <summary>
/// Determines whether a <see cref="ConstantExpression" /> should be converted to a <see cref="InlineQueryRootExpression" />.
/// This handles cases inline expressions whose elements are all constants.
/// </summary>
/// <param name="constantExpression">The constant expression that's a candidate for conversion to a query root.</param>
protected virtual bool ShouldConvertToInlineQueryRoot(ConstantExpression constantExpression)
/// <param name="newArrayExpression">The new array expression that's a candidate for conversion to a query root.</param>
protected virtual bool ShouldConvertToInlineQueryRoot(NewArrayExpression newArrayExpression)
=> false;

/// <summary>
Expand Down
11 changes: 5 additions & 6 deletions test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public PrimitiveCollectionsQuerySqlServerTest(PrimitiveCollectionsQuerySqlServer
: base(fixture)
{
Fixture.TestSqlLoggerFactory.Clear();
// Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

public override async Task Inline_collection_of_ints_Contains(bool async)
Expand Down Expand Up @@ -144,18 +144,17 @@ public override async Task Inline_collection_Contains_with_all_parameters(bool a
{
await base.Inline_collection_Contains_with_all_parameters(async);

// See #30732 for making this better

AssertSql(
"""
@__p_0='[2,999]' (Size = 4000)
@__i_0='2'
@__j_1='999'

SELECT [p].[Id], [p].[Bool], [p].[Bools], [p].[DateTime], [p].[DateTimes], [p].[Enum], [p].[Enums], [p].[Int], [p].[Ints], [p].[NullableInt], [p].[NullableInts], [p].[String], [p].[Strings]
FROM [PrimitiveCollectionsEntity] AS [p]
WHERE EXISTS (
SELECT 1
FROM OpenJson(@__p_0) AS [p0]
WHERE CAST([p0].[value] AS int) = [p].[Id])
FROM (VALUES (@__i_0), (@__j_1)) AS [v]([Value]) -- NO.
WHERE [v].[Value] = [p].[Id])
""");
}

Expand Down

0 comments on commit db7ec78

Please sign in to comment.