TypeMapping bug in translation plugin

[virzak]

Calling code that causes the issue. Cast to long is the problem, works fine without it:

context.Blogs.Select(b => EF.Functions.RowNumber((long)b.BlogId)).First();

Translator:

case nameof(DbFunctionsExtensions.RowNumber):
    var ordeyBy = arguments[1];
    return new RowNumberExpression(null, new OrderingExpression[] { new OrderingExpression(ordeyBy, true) }, RelationalTypeMapping.NullMapping);

Please run typemapping-bug-repro branch of my samples fork to reproduce:
https://github.com/virzak/EntityFramework.Docs/tree/typemapping-bug-repro/samples/core/SqlServer

Stack traces

System.InvalidOperationException
  HResult=0x80131509
  Message=Expression 'Convert(b.BlogId)' in the SQL tree does not have a type mapping assigned.
  Source=Microsoft.EntityFrameworkCore.Relational
  StackTrace:
   at Microsoft.EntityFrameworkCore.Query.RelationalSqlTranslatingExpressionVisitor.SqlTypeMappingVerifyingExpressionVisitor.VisitExtension(Expression extensionExpression) in /_/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs:line 1468
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs:line 163
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 35
   at Microsoft.EntityFrameworkCore.Query.SqlExpressions.OrderingExpression.VisitChildren(ExpressionVisitor visitor) in /_/src/EFCore.Relational/Query/SqlExpressions/OrderingExpression.cs:line 57
   at System.Linq.Expressions.ExpressionVisitor.VisitExtension(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 282
   at Microsoft.EntityFrameworkCore.Query.RelationalSqlTranslatingExpressionVisitor.SqlTypeMappingVerifyingExpressionVisitor.VisitExtension(Expression extensionExpression) in /_/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs:line 1468
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs:line 163
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 35
   at Microsoft.EntityFrameworkCore.Query.SqlExpressions.RowNumberExpression.VisitChildren(ExpressionVisitor visitor) in /_/src/EFCore.Relational/Query/SqlExpressions/RowNumberExpression.cs:line 69
   at System.Linq.Expressions.ExpressionVisitor.VisitExtension(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 282
   at Microsoft.EntityFrameworkCore.Query.RelationalSqlTranslatingExpressionVisitor.SqlTypeMappingVerifyingExpressionVisitor.VisitExtension(Expression extensionExpression) in /_/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs:line 1468
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs:line 163
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 35
   at Microsoft.EntityFrameworkCore.Query.RelationalSqlTranslatingExpressionVisitor.TranslateInternal(Expression expression) in /_/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs:line 143
   at Microsoft.EntityFrameworkCore.Query.RelationalSqlTranslatingExpressionVisitor.Translate(Expression expression) in /_/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs:line 117
   at Microsoft.EntityFrameworkCore.Query.Internal.RelationalProjectionBindingExpressionVisitor.Visit(Expression expression) in /_/src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs:line 220
   at Microsoft.EntityFrameworkCore.Query.Internal.RelationalProjectionBindingExpressionVisitor.Translate(SelectExpression selectExpression, Expression expression) in /_/src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs:line 72
   at Microsoft.EntityFrameworkCore.Query.RelationalQueryableMethodTranslatingExpressionVisitor.TranslateSelect(ShapedQueryExpression source, LambdaExpression selector) in /_/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs:line 849
   at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MethodCallExpression.cs:line 108
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 35
   at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MethodCallExpression.cs:line 108
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) in /_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ExpressionVisitor.cs:line 35
   at Microsoft.EntityFrameworkCore.Query.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass9_0`1.<Execute>b__0()
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression)
   at System.Linq.Queryable.First[TSource](IQueryable`1 source)
   at SqlServer.Plugin.CustomPlugin.Run() in C:\Users\virzak\Code\Examples\EntityFramework.Docs\samples\core\SqlServer\Plugin\CustomPlugin.cs:line 26
   at SqlServer.Program.Main() in C:\Users\virzak\Code\Examples\EntityFramework.Docs\samples\core\SqlServer\Program.cs:line 7

EF Core version: 6
Database provider: SqlServer
Target framework: .NET 6.0
Operating system: Windows 11
IDE: Visual Studio 2022 17.1.0 Preview 1.1

[smitpatel]

This is bug in the plug-in. Convert(b.BlogId) is applying conversion on BlogId column. The type mapping post conversion is supposed to be assigned. In this case, since the plug-in is putting it inside OrderingExpression, plug-in is responsible to figure out appropriate type mapping and assign it to the SqlExpression before passing it into ctor of OrderingExpression.

[virzak]

But it isn't the plug-in that's constructing the original Convert(b.BlogId) expression.

One workaround is to break down the SqlUnaryExpression doing the conversion and reconstruct it with the appropriate TypeMapping.

private SqlExpression AdjustConversion(SqlExpression sqlExpression)
{
   if (sqlExpression is SqlUnaryExpression { OperatorType: ExpressionType.Convert, TypeMapping: null } sqlUnaryExpression)
   {
      var mapping = _typeMappingSource.FindMapping(sqlExpression.Type);

      if (mapping is not null)
         return new SqlUnaryExpression(sqlUnaryExpression.OperatorType, sqlUnaryExpression.Operand, sqlUnaryExpression.Type, mapping);
   }

   return sqlExpression;
}

But it will only work if SqlUnaryExpression is the topmost expression and the following won't work:

.RowNumber((long)b.BlogId)+1)

I don't suppose that the plugin developer is expected to go down the tree recursively to break down and construct everything again...

[roji]

@virzak this isn't about recursively breaking down anything. Translators have the responsibility of producing a tree where type mappings are set on the nodes, whereas you're returning nodes with null mappings (source).

The important point is that a node's mapping isn't always determined based on what it contains, but also on what contains it. For example, in the expression ctx.Blogs.Where(b => b.Name == "Foo"), the type mapping of the Foo constant is inferred from the Name property on the other side of the equality operator.

In your specific case with RowNumber, I don't think there's any inference involved. If that's the case and the type mapping should solely be determined by what's inside the node, then you need to tell EF Core to calculate that by calling SqlExpressionFactory.ApplyDefaultTypeMapping. That method basically sets the type mapping to correspond to the CLR type of the expression node (unless it already has a type mapping).

I suggest looking at some translation code in one of the providers to see how it all works.