Skip to content

Commit

Permalink
Query: Make translation of Coalesce SqlFunction
Browse files Browse the repository at this point in the history 
Resolves #19227
  • Loading branch information
@smitpatel
smitpatel committed Jan 3, 2020
1 parent bfd0d0e commit 78365b3
Show file tree
Hide file tree
Showing 11 changed files with 87 additions and 94 deletions.
9 changes: 0 additions & 9 deletions src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,6 @@ SqlBinaryExpression And(
SqlBinaryExpression Or(
[NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] CoreTypeMapping typeMapping = null);

/// <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>
SqlBinaryExpression Coalesce(
[NotNull] SqlExpression left, [NotNull] SqlExpression right, [CanBeNull] CoreTypeMapping typeMapping = null);

/// <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
Expand Down
5 changes: 1 addition & 4 deletions src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,7 @@ public class QuerySqlGenerator : SqlExpressionVisitor
// Unary
{ ExpressionType.UnaryPlus, "+" },
{ ExpressionType.Negate, "-" },
{ ExpressionType.Not, "~" },

// Others
{ ExpressionType.Coalesce, " ?? " }
{ ExpressionType.Not, "~" }
};

/// <summary>
Expand Down
7 changes: 1 addition & 6 deletions src/EFCore.Cosmos/Query/Internal/SqlBinaryExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ public class SqlBinaryExpression : SqlExpression
ExpressionType.Equal,
ExpressionType.NotEqual,
ExpressionType.ExclusiveOr,
ExpressionType.Coalesce,
ExpressionType.RightShift,
ExpressionType.LeftShift
};
Expand Down Expand Up @@ -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;
}

/// <summary>
Expand Down
10 changes: 0 additions & 10 deletions src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
{
Expand Down Expand Up @@ -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);

/// <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>
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)
{
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/ISqlExpressionFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 17 additions & 0 deletions src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
Expand Down
32 changes: 20 additions & 12 deletions src/EFCore.Relational/Query/Internal/SqlExpressionOptimizingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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;
}
Expand Down
57 changes: 21 additions & 36 deletions src/EFCore.Relational/Query/QuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
12 changes: 7 additions & 5 deletions src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
22 changes: 18 additions & 4 deletions src/EFCore.Relational/Query/SqlExpressionFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
{
Expand Down Expand Up @@ -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<SqlExpression>()
{
ApplyTypeMapping(left, inferredTypeMapping),
ApplyTypeMapping(right, inferredTypeMapping)
};

return SqlFunctionExpression.Create(
"COALESCE",
typeMappedArguments,
resultType,
inferredTypeMapping);
}

public virtual SqlUnaryExpression MakeUnary(
Expand Down Expand Up @@ -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(
Expand Down
8 changes: 1 addition & 7 deletions src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ public class SqlBinaryExpression : SqlExpression
ExpressionType.Equal,
ExpressionType.NotEqual,
//ExpressionType.ExclusiveOr,
ExpressionType.Coalesce
//ExpressionType.ArrayIndex,
//ExpressionType.RightShift,
//ExpressionType.LeftShift,
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 78365b3

Please sign in to comment.