From 78365b38e13ea99bf68bd78aa877395b09a9342d Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Tue, 10 Dec 2019 16:26:47 -0800 Subject: [PATCH] Query: Make translation of Coalesce SqlFunction Resolves #19227 --- .../Query/Internal/ISqlExpressionFactory.cs | 9 --- .../Query/Internal/QuerySqlGenerator.cs | 5 +- .../Query/Internal/SqlBinaryExpression.cs | 7 +-- .../Query/Internal/SqlExpressionFactory.cs | 10 ---- .../Query/ISqlExpressionFactory.cs | 2 +- ...NullSemanticsRewritingExpressionVisitor.cs | 17 ++++++ ...qlExpressionOptimizingExpressionVisitor.cs | 32 +++++++---- .../Query/QuerySqlGenerator.cs | 57 +++++++------------ ...lationalSqlTranslatingExpressionVisitor.cs | 12 ++-- .../Query/SqlExpressionFactory.cs | 22 +++++-- .../SqlExpressions/SqlBinaryExpression.cs | 8 +-- 11 files changed, 87 insertions(+), 94 deletions(-) diff --git a/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs b/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs index 598142f9561..a8ff2add268 100644 --- a/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs +++ b/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs @@ -181,15 +181,6 @@ SqlBinaryExpression And( SqlBinaryExpression Or( [NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] CoreTypeMapping typeMapping = null); - /// - /// 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. - /// - SqlBinaryExpression Coalesce( - [NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] CoreTypeMapping typeMapping = null); - /// /// 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 diff --git a/src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs b/src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs index 11656852406..6cdd2ad1e46 100644 --- a/src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs +++ b/src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs @@ -60,10 +60,7 @@ public class QuerySqlGenerator : SqlExpressionVisitor // Unary { ExpressionType.UnaryPlus, "+" }, { ExpressionType.Negate, "-" }, - { ExpressionType.Not, "~" }, - - // Others - { ExpressionType.Coalesce, " ?? " } + { ExpressionType.Not, "~" } }; /// diff --git a/src/EFCore.Cosmos/Query/Internal/SqlBinaryExpression.cs b/src/EFCore.Cosmos/Query/Internal/SqlBinaryExpression.cs index 4ae45cecc76..49356b27738 100644 --- a/src/EFCore.Cosmos/Query/Internal/SqlBinaryExpression.cs +++ b/src/EFCore.Cosmos/Query/Internal/SqlBinaryExpression.cs @@ -37,7 +37,6 @@ public class SqlBinaryExpression : SqlExpression ExpressionType.Equal, ExpressionType.NotEqual, ExpressionType.ExclusiveOr, - ExpressionType.Coalesce, ExpressionType.RightShift, ExpressionType.LeftShift }; @@ -160,12 +159,8 @@ public override void Print(ExpressionPrinter expressionPrinter) { expressionPrinter.Append(")"); } - } - private bool RequiresBrackets(SqlExpression expression) - { - return expression is SqlBinaryExpression sqlBinary - && sqlBinary.OperatorType != ExpressionType.Coalesce; + static bool RequiresBrackets(SqlExpression expression) => expression is SqlBinaryExpression; } /// diff --git a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs index 5b122e47332..5956c5c850d 100644 --- a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs +++ b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs @@ -179,7 +179,6 @@ private SqlExpression ApplyTypeMappingOnSqlBinary( case ExpressionType.Modulo: case ExpressionType.LeftShift: case ExpressionType.RightShift: - case ExpressionType.Coalesce: case ExpressionType.And: case ExpressionType.Or: { @@ -373,15 +372,6 @@ public virtual SqlBinaryExpression And(SqlExpression left, SqlExpression right, public virtual SqlBinaryExpression Or(SqlExpression left, SqlExpression right, CoreTypeMapping typeMapping = null) => MakeBinary(ExpressionType.Or, left, right, typeMapping); - /// - /// 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. - /// - public virtual SqlBinaryExpression Coalesce(SqlExpression left, SqlExpression right, CoreTypeMapping typeMapping = null) - => MakeBinary(ExpressionType.Coalesce, left, right, typeMapping); - private SqlUnaryExpression MakeUnary( ExpressionType operatorType, SqlExpression operand, Type type, CoreTypeMapping typeMapping = null) { diff --git a/src/EFCore.Relational/Query/ISqlExpressionFactory.cs b/src/EFCore.Relational/Query/ISqlExpressionFactory.cs index 10348045362..955e4857bbe 100644 --- a/src/EFCore.Relational/Query/ISqlExpressionFactory.cs +++ b/src/EFCore.Relational/Query/ISqlExpressionFactory.cs @@ -79,7 +79,7 @@ SqlBinaryExpression Or( [NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] RelationalTypeMapping typeMapping = null); // Other - SqlBinaryExpression Coalesce( + SqlFunctionExpression Coalesce( [NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] RelationalTypeMapping typeMapping = null); SqlUnaryExpression IsNull([NotNull] SqlExpression operand); diff --git a/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs index bd97aead1a3..69bf5410321 100644 --- a/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs @@ -624,6 +624,23 @@ protected override Expression VisitSqlFunction(SqlFunctionExpression sqlFunction var canOptimize = _canOptimize; _canOptimize = false; + if (sqlFunctionExpression.IsBuiltIn + && string.Equals(sqlFunctionExpression.Name, "COALESCE", StringComparison.OrdinalIgnoreCase)) + { + _isNullable = false; + var newLeft = (SqlExpression)Visit(sqlFunctionExpression.Arguments[0]); + var leftNullable = _isNullable; + + _isNullable = false; + var newRight = (SqlExpression)Visit(sqlFunctionExpression.Arguments[1]); + var rightNullable = _isNullable; + + _isNullable = leftNullable && rightNullable; + _canOptimize = canOptimize; + + return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, new[] { newLeft, newRight }); + } + var newInstance = (SqlExpression)Visit(sqlFunctionExpression.Instance); var newArguments = new SqlExpression[sqlFunctionExpression.Arguments.Count]; for (var i = 0; i < newArguments.Length; i++) diff --git a/src/EFCore.Relational/Query/Internal/SqlExpressionOptimizingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/SqlExpressionOptimizingExpressionVisitor.cs index 1e555aa198d..eb3aa7a81c6 100644 --- a/src/EFCore.Relational/Query/Internal/SqlExpressionOptimizingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/SqlExpressionOptimizingExpressionVisitor.cs @@ -250,9 +250,6 @@ private SqlExpression SimplifyNullNotNullExpression( // in general: // binaryOp(a, b) == null -> a == null || b == null // binaryOp(a, b) != null -> a != null && b != null - // for coalesce: - // (a ?? b) == null -> a == null && b == null - // (a ?? b) != null -> a != null || b != null // for AndAlso, OrElse we can't do this optimization // we could do something like this, but it seems too complicated: // (a && b) == null -> a == null && b != 0 || a != 0 && b == null @@ -262,15 +259,7 @@ private SqlExpression SimplifyNullNotNullExpression( var newLeft = SimplifyNullNotNullExpression(operatorType, sqlBinaryOperand.Left, typeof(bool), typeMapping); var newRight = SimplifyNullNotNullExpression(operatorType, sqlBinaryOperand.Right, typeof(bool), typeMapping); - return sqlBinaryOperand.OperatorType == ExpressionType.Coalesce - ? SimplifyLogicalSqlBinaryExpression( - operatorType == ExpressionType.Equal - ? ExpressionType.AndAlso - : ExpressionType.OrElse, - newLeft, - newRight, - typeMapping) - : SimplifyLogicalSqlBinaryExpression( + return SimplifyLogicalSqlBinaryExpression( operatorType == ExpressionType.Equal ? ExpressionType.OrElse : ExpressionType.AndAlso, @@ -279,6 +268,25 @@ private SqlExpression SimplifyNullNotNullExpression( typeMapping); } break; + + case SqlFunctionExpression sqlFunctionExpression + when sqlFunctionExpression.IsBuiltIn + && string.Equals("COALESCE", sqlFunctionExpression.Name, StringComparison.OrdinalIgnoreCase): + // for coalesce: + // (a ?? b) == null -> a == null && b == null + // (a ?? b) != null -> a != null || b != null + var leftArgument = SimplifyNullNotNullExpression( + operatorType, sqlFunctionExpression.Arguments[0], typeof(bool), typeMapping); + var rightArgument = SimplifyNullNotNullExpression( + operatorType, sqlFunctionExpression.Arguments[1], typeof(bool), typeMapping); + + return SimplifyLogicalSqlBinaryExpression( + operatorType == ExpressionType.Equal + ? ExpressionType.AndAlso + : ExpressionType.OrElse, + leftArgument, + rightArgument, + typeMapping); } break; } diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs index 37300d8749b..0e94e805dc5 100644 --- a/src/EFCore.Relational/Query/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/QuerySqlGenerator.cs @@ -375,56 +375,41 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres { Check.NotNull(sqlBinaryExpression, nameof(sqlBinaryExpression)); - if (sqlBinaryExpression.OperatorType == ExpressionType.Coalesce) + var requiresBrackets = RequiresBrackets(sqlBinaryExpression.Left); + + if (requiresBrackets) { - _relationalCommandBuilder.Append("COALESCE("); - Visit(sqlBinaryExpression.Left); - _relationalCommandBuilder.Append(", "); - Visit(sqlBinaryExpression.Right); - _relationalCommandBuilder.Append(")"); + _relationalCommandBuilder.Append("("); } - else - { - var requiresBrackets = RequiresBrackets(sqlBinaryExpression.Left); - if (requiresBrackets) - { - _relationalCommandBuilder.Append("("); - } + Visit(sqlBinaryExpression.Left); - Visit(sqlBinaryExpression.Left); - - if (requiresBrackets) - { - _relationalCommandBuilder.Append(")"); - } + if (requiresBrackets) + { + _relationalCommandBuilder.Append(")"); + } - _relationalCommandBuilder.Append(GenerateOperator(sqlBinaryExpression)); + _relationalCommandBuilder.Append(GenerateOperator(sqlBinaryExpression)); - requiresBrackets = RequiresBrackets(sqlBinaryExpression.Right); + requiresBrackets = RequiresBrackets(sqlBinaryExpression.Right); - if (requiresBrackets) - { - _relationalCommandBuilder.Append("("); - } + if (requiresBrackets) + { + _relationalCommandBuilder.Append("("); + } - Visit(sqlBinaryExpression.Right); + Visit(sqlBinaryExpression.Right); - if (requiresBrackets) - { - _relationalCommandBuilder.Append(")"); - } + if (requiresBrackets) + { + _relationalCommandBuilder.Append(")"); } return sqlBinaryExpression; } - private bool RequiresBrackets(SqlExpression expression) - { - return expression is SqlBinaryExpression sqlBinary - && sqlBinary.OperatorType != ExpressionType.Coalesce - || expression is LikeExpression; - } + private static bool RequiresBrackets(SqlExpression expression) + => expression is SqlBinaryExpression || expression is LikeExpression; protected override Expression VisitSqlConstant(SqlConstantExpression sqlConstantExpression) { diff --git a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs index 3c03c11b4e8..7effe1b62dd 100644 --- a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs @@ -512,11 +512,13 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression) return TranslationFailed(binaryExpression.Left, Visit(left), out var sqlLeft) || TranslationFailed(binaryExpression.Right, Visit(right), out var sqlRight) ? null - : SqlExpressionFactory.MakeBinary( - binaryExpression.NodeType, - sqlLeft, - sqlRight, - null); + : binaryExpression.NodeType == ExpressionType.Coalesce + ? SqlExpressionFactory.Coalesce(sqlLeft, sqlRight) + : (Expression)SqlExpressionFactory.MakeBinary( + binaryExpression.NodeType, + sqlLeft, + sqlRight, + null); } private SqlConstantExpression GetConstantOrNull(Expression expression) diff --git a/src/EFCore.Relational/Query/SqlExpressionFactory.cs b/src/EFCore.Relational/Query/SqlExpressionFactory.cs index b31644d24f8..9703701c969 100644 --- a/src/EFCore.Relational/Query/SqlExpressionFactory.cs +++ b/src/EFCore.Relational/Query/SqlExpressionFactory.cs @@ -180,7 +180,6 @@ private SqlExpression ApplyTypeMappingOnSqlBinary( case ExpressionType.Multiply: case ExpressionType.Divide: case ExpressionType.Modulo: - case ExpressionType.Coalesce: case ExpressionType.And: case ExpressionType.Or: { @@ -358,12 +357,27 @@ public virtual SqlBinaryExpression Or(SqlExpression left, SqlExpression right, R return MakeBinary(ExpressionType.Or, left, right, typeMapping); } - public virtual SqlBinaryExpression Coalesce(SqlExpression left, SqlExpression right, RelationalTypeMapping typeMapping = null) + public virtual SqlFunctionExpression Coalesce(SqlExpression left, SqlExpression right, RelationalTypeMapping typeMapping = null) { Check.NotNull(left, nameof(left)); Check.NotNull(right, nameof(right)); - return MakeBinary(ExpressionType.Coalesce, left, right, typeMapping); + var resultType = right.Type; + var inferredTypeMapping = typeMapping + ?? ExpressionExtensions.InferTypeMapping(left, right) + ?? _typeMappingSource.FindMapping(resultType); + + var typeMappedArguments = new List() + { + ApplyTypeMapping(left, inferredTypeMapping), + ApplyTypeMapping(right, inferredTypeMapping) + }; + + return SqlFunctionExpression.Create( + "COALESCE", + typeMappedArguments, + resultType, + inferredTypeMapping); } public virtual SqlUnaryExpression MakeUnary( @@ -677,7 +691,7 @@ private void AddConditions( if (sharingTypes.Count > 0) { - bool discriminatorAdded = AddDiscriminatorCondition(selectExpression, entityType); + var discriminatorAdded = AddDiscriminatorCondition(selectExpression, entityType); var linkingFks = entityType.GetRootType().FindForeignKeys(entityType.FindPrimaryKey().Properties) .Where( diff --git a/src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs index 0b52afe55b0..6e753874a4a 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs @@ -31,7 +31,6 @@ public class SqlBinaryExpression : SqlExpression ExpressionType.Equal, ExpressionType.NotEqual, //ExpressionType.ExclusiveOr, - ExpressionType.Coalesce //ExpressionType.ArrayIndex, //ExpressionType.RightShift, //ExpressionType.LeftShift, @@ -116,13 +115,8 @@ public override void Print(ExpressionPrinter expressionPrinter) { expressionPrinter.Append(")"); } - } - private bool RequiresBrackets(SqlExpression expression) - { - return expression is SqlBinaryExpression sqlBinary - && sqlBinary.OperatorType != ExpressionType.Coalesce - || expression is LikeExpression; + static bool RequiresBrackets(SqlExpression expression) => expression is SqlBinaryExpression || expression is LikeExpression; } public override bool Equals(object obj)