Fix to #26344 - SqlNullabilityProcessor and COALESCE with more than two arguments #26419
Conversation
|
new version up @smitpatel |
src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs
Show resolved
Hide resolved
| ? new SqlFunctionExpression( | ||
| sqlFunctionExpression.Name, | ||
| distinctArguments, | ||
| sqlFunctionExpression.IsNullable, | ||
| argumentsPropagateNullability: distinctArguments.Select(a => false).ToArray(), | ||
| sqlFunctionExpression.Type, | ||
| sqlFunctionExpression.TypeMapping) |
There was a problem hiding this comment.
In my opinion, it is better to define and use a new
SqlExpression SqlExpressionFactory::Coalesce(params SqlExpression[] parameters);instead direct creation of SqlFunctionExpression.
There was a problem hiding this comment.
@dmitry-lipetsk coalesce will get to us in form of binary expression so this logic is necessary as well.
We could add a SqlExpressionFactory overload that takes multiple args, but I'm not sure how it would be used - do you have a scenario in mind?
One thing that I can think of is constructing sqlexpression tree manually in the HasTranslation, but for the translation we take care of type mappings so users can call ctor directly there also.
Main advantage of using expression factory over the ctor directly (the way I see it) is that the former takes care of type mappings for you. However, after the initial translation the type mappings are filled in, so there's no need to use the factory.
There was a problem hiding this comment.
Hello @maumar,
For COALESCE and all another built-in, SQL-function I use own classes based SqlFunctionExpression. This is help to resolve some problems with SQL-tree transformation (through Update and VisitChildren methods) and improve generation of SQL text.
Usage of SqlExpressionFactory.Coalesce instead of direct creation of SqlFunctionExpression allows to save a custom expression class for COALESCE.
In case of COALESCE, replacing to standard SqFunctionlExpression not critical for me.
At current time I nothing know about HasTranslation :)
I finished implementation of translation to SQL and initiate implementation of Migrations support.
There was a problem hiding this comment.
Given the coalesce in LINQ will come with 2 arguments, there is no particular value in relational layer to have another overload. Providers who translates Coalesce with multiple arguments directly can add overload in provider specific ISqlExpressionFactory as needed.
…wo arguments Nested coalesce ops are converted to coalesce with multiple arguments. Fixing null semantics logic that only assumed coalesce function would have two arguments Fixes #26344
Nested coalesce ops are converted to coalesce with multiple arguments. Fixing null semantics logic that only assumed coalesce function would have two arguments
Fixes #26344