Skip to content
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

[WIP] Handle first class spans #35279

Closed
wants to merge 3 commits into from
Closed
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 @@ -44,6 +44,13 @@ public ByteArraySequenceEqualTranslator(ISqlExpressionFactory sqlExpressionFacto
return _sqlExpressionFactory.Equal(arguments[0], arguments[1]);
}

if (method.IsGenericMethod
&& MemoryExtensionsMethods.SequenceEqual.Contains(method.GetGenericMethodDefinition())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisJollyAU thanks for working on this, and sorry that I didn't have time to look earlier.

As per dotnet/runtime#109757 (comment) - which I hope you agree is a good approach to handle this - I think we'd be normalizing away method calls like MemoryExtensions.Contains - replacing them with corresponding non-Span calls - very early in the pipeline, in ExpressionTreeFuncletizer. If we go down this road, no later part of the query pipeline will ever see these MemoryExtensions methods.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did see that. It is one way to do it and was the alternative I considered. Not sure what the performance cost is though of rewriting the query away from span (implicit) back to normal versus handling it direct.

Note that I actually have it that there is only interpretation being done with very minimal changes to the funcletizer. Nothing is getting the full compilation. It just gets directly picked in in the translator mostly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, take for example this

Span<string> data = new[] { "ALFKI" + "SomeVariable", "ANATR" + "SomeVariable", "ALFKI" + "X" };
var someVariable = "SomeVariable";

return AssertQuery(
    async,
    ss => ss.Set<Customer>().Where(c => data.Contains(c.CustomerID + someVariable)));

Currently this doesn't even compile (Expression tree cannot contain value of ref struct or restricted type) but should that support get added (that other thread has mentioned a couple of issues in order to get it working), rewriting it away would not be able to work. This handling it direct in the translator side would handle it (without much change if any I think)

Copy link
Member

@roji roji Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the performance cost is though of rewriting the query away from span (implicit) back to normal versus handling it direct.

What I had in mind would be to do the rewriting as part of the funcletization pass, rather than adding an additional pass just for that; if we do it that way, the perf impact should be completely negligible, I think.

But more important: when the funcletizer identifies a tree fragment that can be client-evaluated, it does that and embeds the result either as a constant or as a parameter. If such an evaluatable tree fragment happens to contains a Contains, won't that now start throwing, since client evaluation involves going through the LINQ interpreter? Am I missing something here?

if so, then the funcletizer must do the substitution early (i.e. remove any Span-based method overloads), because it has to happen before the LINQ interpreter is possibly used.

Currently this doesn't even compile [...]

Right, but that seems pretty orthogonal to the discussion (and not necessarily extremely important). I indeed don't think the C# compiler will allow ref structs inside LINQ expression trees any time soon; that's also not a change from 9 to 10 - it has never been allowed, and as far as I know nobody has ever complained about it (there's no real reason to use a Span variable rather than an array when querying like this).

&& arguments[0].Type == typeof(byte[]))
{
return _sqlExpressionFactory.Equal(arguments[0], arguments[1]);
}

return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ public ContainsTranslator(ISqlExpressionFactory sqlExpressionFactory)

// Identify static Enumerable.Contains and instance List.Contains
if (method.IsGenericMethod
&& method.GetGenericMethodDefinition() == EnumerableMethods.Contains
&& (method.GetGenericMethodDefinition() == EnumerableMethods.Contains || MemoryExtensionsMethods.Contains.Contains(method.GetGenericMethodDefinition()))
&& ValidateValues(arguments[0]))
{
(itemExpression, valuesExpression) = (RemoveObjectConvert(arguments[1]), arguments[0]);
}

if (arguments.Count == 1
&& method.IsContainsMethod()
&& (method.IsContainsMethod())
&& instance != null
&& ValidateValues(instance))
{
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Text;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;
using Microsoft.EntityFrameworkCore.SqlServer.Internal;
using ExpressionExtensions = Microsoft.EntityFrameworkCore.Query.ExpressionExtensions;

namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
Expand Down Expand Up @@ -267,6 +268,19 @@ bool TryTranslateStartsEndsWithContains(
StartsEndsWithContains methodType,
[NotNullWhen(true)] out SqlExpression? translation)
{
if (pattern is UnaryExpression
{
NodeType:ExpressionType.Convert
} unary)
{
if (unary.Type == typeof(Span<string>))
{
pattern = unary.Operand;
}
}

instance = RemoveConvert(instance);
pattern = RemoveConvert(pattern);
if (Visit(instance) is not SqlExpression translatedInstance
|| Visit(pattern) is not SqlExpression translatedPattern)
{
Expand Down Expand Up @@ -648,4 +662,125 @@ private static bool TranslationFailed(Expression? original, Expression? translat

private static string? GetProviderType(SqlExpression expression)
=> expression.TypeMapping?.StoreType;

[return: NotNullIfNotNull(nameof(expression))]
private static Expression? RemoveConvert(Expression? expression)
=> expression is UnaryExpression { NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked } unaryExpression
? RemoveConvert(unaryExpression.Operand)
: expression is MethodCallExpression { Method.Name: "op_Implicit" } methodCallExpression ?
RemoveConvert(methodCallExpression.Arguments[0])
: expression;

/// <summary>
/// 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.
/// </summary>
protected override ShapedQueryExpression? TranslatePrimitiveCollection(
SqlExpression sqlExpression,
IProperty? property,
string tableAlias)
{
if (_sqlServerSingletonOptions.EngineType == SqlServerEngineType.SqlServer
&& _sqlServerSingletonOptions.SqlServerCompatibilityLevel < 130)
{
AddTranslationErrorDetails(
SqlServerStrings.CompatibilityLevelTooLowForScalarCollections(_sqlServerSingletonOptions.SqlServerCompatibilityLevel));

return null;
}

if (_sqlServerSingletonOptions.EngineType == SqlServerEngineType.AzureSql
&& _sqlServerSingletonOptions.AzureSqlCompatibilityLevel < 130)
{
AddTranslationErrorDetails(
SqlServerStrings.CompatibilityLevelTooLowForScalarCollections(_sqlServerSingletonOptions.AzureSqlCompatibilityLevel));

return null;
}

// Generate the OPENJSON function expression, and wrap it in a SelectExpression.

// Note that where the elementTypeMapping is known (i.e. collection columns), we immediately generate OPENJSON with a WITH clause
// (i.e. with a columnInfo), which determines the type conversion to apply to the JSON elements coming out.
// For parameter collections, the element type mapping will only be inferred and applied later (see
// SqlServerInferredTypeMappingApplier below), at which point the we'll apply it to add the WITH clause.
var elementTypeMapping = (RelationalTypeMapping?)sqlExpression.TypeMapping?.ElementTypeMapping;

var openJsonExpression = elementTypeMapping is null
? new SqlServerOpenJsonExpression(tableAlias, sqlExpression)
: new SqlServerOpenJsonExpression(
tableAlias, sqlExpression,
columnInfos: new[]
{
new SqlServerOpenJsonExpression.ColumnInfo
{
Name = "value",
TypeMapping = elementTypeMapping,
Path = []
}
});

var elementClrType = sqlExpression.Type.GetSequenceType();

// If this is a collection property, get the element's nullability out of metadata. Otherwise, this is a parameter property, in
// which case we only have the CLR type (note that we cannot produce different SQLs based on the nullability of an *element* in
// a parameter collection - our caching mechanism only supports varying by the nullability of the parameter itself (i.e. the
// collection).
var isElementNullable = property?.GetElementType()!.IsNullable;

var keyColumnTypeMapping = _typeMappingSource.FindMapping("nvarchar(4000)")!;
#pragma warning disable EF1001 // Internal EF Core API usage.
var selectExpression = new SelectExpression(
[openJsonExpression],
new ColumnExpression(
"value",
tableAlias,
elementClrType.UnwrapNullableType(),
elementTypeMapping,
isElementNullable ?? elementClrType.IsNullableType()),
identifier:
[
(new ColumnExpression("key", tableAlias, typeof(string), keyColumnTypeMapping, nullable: false),
keyColumnTypeMapping.Comparer)
],
_queryCompilationContext.SqlAliasManager);
#pragma warning restore EF1001 // Internal EF Core API usage.

// OPENJSON doesn't guarantee the ordering of the elements coming out; when using OPENJSON without WITH, a [key] column is returned
// with the JSON array's ordering, which we can ORDER BY; this option doesn't exist with OPENJSON with WITH, unfortunately.
// However, OPENJSON with WITH has better performance, and also applies JSON-specific conversions we cannot be done otherwise
// (e.g. OPENJSON with WITH does base64 decoding for VARBINARY).
// Here we generate OPENJSON with WITH, but also add an ordering by [key] - this is a temporary invalid representation.
// In SqlServerQueryTranslationPostprocessor, we'll post-process the expression; if the ORDER BY was stripped (e.g. because of
// IN, EXISTS or a set operation), we'll just leave the OPENJSON with WITH. If not, we'll convert the OPENJSON with WITH to an
// OPENJSON without WITH.
// Note that the OPENJSON 'key' column is an nvarchar - we convert it to an int before sorting.
selectExpression.AppendOrdering(
new OrderingExpression(
_sqlExpressionFactory.Convert(
selectExpression.CreateColumnExpression(
openJsonExpression,
"key",
typeof(string),
typeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"),
columnNullable: false),
typeof(int),
_typeMappingSource.FindMapping(typeof(int))),
ascending: true));

var shaperExpression = (Expression)new ProjectionBindingExpression(
selectExpression, new ProjectionMember(), elementClrType.MakeNullable());
if (shaperExpression.Type != elementClrType)
{
Check.DebugAssert(
elementClrType.MakeNullable() == shaperExpression.Type,
"expression.Type must be nullable of targetType");

shaperExpression = Expression.Convert(shaperExpression, elementClrType);
}

return new ShapedQueryExpression(selectExpression, shaperExpression);
}
}
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 Microsoft.EntityFrameworkCore.Query.SqlExpressions;
using MemoryExtensions = System.MemoryExtensions;

// ReSharper disable once CheckNamespace
namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
Expand Down Expand Up @@ -59,19 +60,41 @@ public SqlServerByteArrayMethodTranslator(ISqlExpressionFactory sqlExpressionFac
typeof(int)),
_sqlExpressionFactory.Constant(0));
}
}

if (methodDefinition.Equals(EnumerableMethods.FirstWithoutPredicate)
&& arguments[0].Type == typeof(byte[]))
{
return _sqlExpressionFactory.Convert(
_sqlExpressionFactory.Function(
"SUBSTRING",
[arguments[0], _sqlExpressionFactory.Constant(1), _sqlExpressionFactory.Constant(1)],
nullable: true,
argumentsPropagateNullability: Statics.TrueArrays[3],
typeof(byte[])),
method.ReturnType);
}
if (method.IsGenericMethod
&& MemoryExtensionsMethods.Contains.Contains(method.GetGenericMethodDefinition())
&& (arguments[0].Type == typeof(Span<byte>) || arguments[0].Type == typeof(byte[])))
{
var source = arguments[0];
var sourceTypeMapping = source.TypeMapping;

var value = arguments[1] is SqlConstantExpression constantValue
? _sqlExpressionFactory.Constant(new[] { (byte)constantValue.Value! }, sourceTypeMapping)
: _sqlExpressionFactory.Convert(arguments[1], typeof(byte[]), sourceTypeMapping);

return _sqlExpressionFactory.GreaterThan(
_sqlExpressionFactory.Function(
"CHARINDEX",
[value, source],
nullable: true,
argumentsPropagateNullability: [true, true],
typeof(int)),
_sqlExpressionFactory.Constant(0));
}

if (method.IsGenericMethod
&& method.GetGenericMethodDefinition().Equals(EnumerableMethods.FirstWithoutPredicate)
&& arguments[0].Type == typeof(byte[]))
{
return _sqlExpressionFactory.Convert(
_sqlExpressionFactory.Function(
"SUBSTRING",
[arguments[0], _sqlExpressionFactory.Constant(1), _sqlExpressionFactory.Constant(1)],
nullable: true,
argumentsPropagateNullability: [true, true, true],
typeof(byte[])),
method.ReturnType);
}

return null;
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore/Query/EvaluatableExpressionFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ public virtual bool IsEvaluatableExpression(Expression expression, IModel model)
|| Equals(method, RandomNextNoArgs)
|| Equals(method, RandomNextOneArg)
|| Equals(method, RandomNextTwoArgs)
|| method.DeclaringType == typeof(DbFunctionsExtensions))
|| method.DeclaringType == typeof(DbFunctionsExtensions)
|| method.Name == "op_Implicit")//should this be not evaluatble?
{
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2043,11 +2043,11 @@ bool PreserveConvertNode(Expression expression)
return Lambda(visited, _contextParameterReplacer.ContextParameterExpression);
}

static Expression RemoveConvert(Expression expression)
static Expression? RemoveConvert(Expression? expression)
=> expression is UnaryExpression { NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked } unaryExpression
? RemoveConvert(unaryExpression.Operand)
: expression;
}
}

switch (expression)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/ExpressionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static LambdaExpression UnwrapLambdaFromQuote(this Expression expression)
return expression;
}

private static Expression RemoveConvert(Expression expression)
private static Expression? RemoveConvert(Expression? expression)
=> expression is UnaryExpression { NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked } unaryExpression
? RemoveConvert(unaryExpression.Operand)
: expression;
Expand Down
Loading