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

Query: Make translation of Coalesce SqlFunction #19270

Merged
merged 1 commit into from
Jan 3, 2020
Merged
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
9 changes: 0 additions & 9 deletions src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs
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);
roji marked this conversation as resolved.
Show resolved Hide resolved

/// <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
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
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
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
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
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
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
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
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
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
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