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

Simplify AND and OR #34133

Merged
merged 3 commits into from
Jul 2, 2024
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
4 changes: 3 additions & 1 deletion src/EFCore.Relational/Query/ISqlExpressionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ public interface ISqlExpressionFactory
/// <param name="left">The left operand of binary operation.</param>
/// <param name="right">The right operand of binary operation.</param>
/// <param name="typeMapping">A type mapping to be assigned to the created expression.</param>
/// <param name="existingExpr">An optional expression that can be re-used if it matches the new expression.</param>
/// <returns>A <see cref="SqlExpression" /> with the given arguments.</returns>
SqlExpression? MakeBinary(
ExpressionType operatorType,
SqlExpression left,
SqlExpression right,
RelationalTypeMapping? typeMapping);
RelationalTypeMapping? typeMapping,
SqlExpression? existingExpr = null);

// Comparison
/// <summary>
Expand Down
90 changes: 87 additions & 3 deletions src/EFCore.Relational/Query/SqlExpressionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,17 @@ private SqlExpression ApplyTypeMappingOnJsonScalar(
ExpressionType operatorType,
SqlExpression left,
SqlExpression right,
RelationalTypeMapping? typeMapping)
RelationalTypeMapping? typeMapping,
SqlExpression? existingExpr = null)
{
switch (operatorType)
{
case ExpressionType.AndAlso:
return ApplyTypeMapping(AndAlso(left, right, existingExpr), typeMapping);
case ExpressionType.OrElse:
return ApplyTypeMapping(OrElse(left, right, existingExpr), typeMapping);
}

if (!SqlBinaryExpression.IsValidOperator(operatorType))
{
return null;
Expand All @@ -411,8 +420,6 @@ private SqlExpression ApplyTypeMappingOnJsonScalar(
case ExpressionType.LessThan:
case ExpressionType.LessThanOrEqual:
case ExpressionType.NotEqual:
case ExpressionType.AndAlso:
case ExpressionType.OrElse:
returnType = typeof(bool);
break;
}
Expand Down Expand Up @@ -449,10 +456,87 @@ public virtual SqlExpression LessThanOrEqual(SqlExpression left, SqlExpression r
public virtual SqlExpression AndAlso(SqlExpression left, SqlExpression right)
=> MakeBinary(ExpressionType.AndAlso, left, right, null)!;

private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpression? existingExpr)
{
// false && x -> false
// x && true -> x
// x && x -> x
if (left is SqlConstantExpression { Value: false }
|| right is SqlConstantExpression { Value: true }
|| left.Equals(right))
Copy link
Member

@roji roji Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranma42 I thought this a bit more over the night.

It's worth considering that left.Equals(right) is a recursive visitation, which is very different from simple pattern matching like checking for false/true constants. In a contrived case where 100 AndAlso expression are being composed over each either, the number of visitations grows very quickly, and this can lead to a compilation performance issue (I say this is contrived, though in some cases where query trees are generated automatically it may happen).

I don't think this is necessarily something we need to worry about right now - SQL expression trees are typically not that deep, and IIUC the problem also existed before when the optimization happened as part of SqlNullabilitryProcessor. But it's something to consider; for example, SqlExpression could cache its hash code when it's first calculated, and our recursive equality logic could simply check that first, and return early rather than recursing.

Let me know if you have any thoughts on this - we can open a separate issue to track this work (which again, I don't think is urgent).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this problem is already present in a few places in the codebase and it is something I would definitely like to improve (and use more 😈 ).
Your suggestion (compute & reuse the hash code) is indeed the most straightforward way to improve it... it would be trivial to implement if the expressions were immutable 😉 #32927.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted #34149 to track this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be trivial to implement if the expressions were immutable

FWIW almost our entire SQL tree is already immutable, with the big exception being SelectExpression (and that's also supposed to be immutable once translation is complete, i.e. in post-processing - though I wouldn't commit to it 100%...). So we should be able to implement the has code optimization today for almost all expression types, and specifically skip SelectExpression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SelectExpression can be a subtree of most expressions (it is used in InExpression) :(
Maybe this is something we want to do on an IR and possibly skip/ignore on C# and SQL expressions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, you're right. I don't think we need to wait on an IR here though - #32927 (full immutability) is something I want us to do for many other reasons, regardless/before an IR.

{
return left;
}
// true && x -> x
// x && false -> false
else if (left is SqlConstantExpression { Value: true } || right is SqlConstantExpression { Value: false })
{
return right;
}
// x is null && x is not null -> false
// x is not null && x is null -> false
else if (left is SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } leftUnary
&& right is SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } rightUnary
&& leftUnary.Operand.Equals(rightUnary.Operand))
{
// the case in which left and right are the same expression is handled above
return Constant(false);
}
else if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.AndAlso } binaryExpr
&& left == binaryExpr.Left
&& right == binaryExpr.Right)
{
return existingExpr;
}
else
{
return new SqlBinaryExpression(ExpressionType.AndAlso, left, right, typeof(bool), null);
}
}

/// <inheritdoc />
public virtual SqlExpression OrElse(SqlExpression left, SqlExpression right)
=> MakeBinary(ExpressionType.OrElse, left, right, null)!;

private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpression? existingExpr = null)
{
// true || x -> true
// x || false -> x
// x || x -> x
if (left is SqlConstantExpression { Value: true }
|| right is SqlConstantExpression { Value: false }
|| left.Equals(right))
{
return left;
}
// false || x -> x
// x || true -> true
else if (left is SqlConstantExpression { Value: false }
|| right is SqlConstantExpression { Value: true })
{
return right;
}
// x is null || x is not null -> true
// x is not null || x is null -> true
else if (left is SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } leftUnary
&& right is SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } rightUnary
&& leftUnary.Operand.Equals(rightUnary.Operand))
{
// the case in which left and right are the same expression is handled above
return Constant(true);
}
else if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binaryExpr
&& left == binaryExpr.Left
&& right == binaryExpr.Right)
{
return existingExpr;
}
else
{
return new SqlBinaryExpression(ExpressionType.OrElse, left, right, typeof(bool), null);
}
}

/// <inheritdoc />
public virtual SqlExpression Add(SqlExpression left, SqlExpression right, RelationalTypeMapping? typeMapping = null)
=> MakeBinary(ExpressionType.Add, left, right, typeMapping)!;
Expand Down
139 changes: 36 additions & 103 deletions src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1358,8 +1358,14 @@ or ExpressionType.LessThan
}

return result is SqlBinaryExpression sqlBinaryResult
&& sqlBinaryExpression.OperatorType is ExpressionType.AndAlso or ExpressionType.OrElse
? SimplifyLogicalSqlBinaryExpression(sqlBinaryResult)
&& sqlBinaryResult.OperatorType is ExpressionType.AndAlso or ExpressionType.OrElse
? _sqlExpressionFactory.MakeBinary( // invoke MakeBinary simplifications
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One slight disadvantage here is that this always reconstructs the SqlBinaryExpression (allocation), regardless of whether it's simplified or not. We could refactor a bit and have a SimplifyBinary method on SqlExpressionFactory that simply returns the original expression if it couldn't be optimized (reusing the actual code internally from MakeBinary().

It might be a bit of a micro-optimization, but it might be worth doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is something I am hitting with the branch on top of this (you might guess... it's NOT 😅 (and it combines much better than I expected with this branch 🤯)).
I will further investigate options to support an Update-like pattern with no (additional) allocations.

In the meantime, I might try a middle-ground solution, i.e. creating the new expression and if it is .Equals to the old one, propagate the old one. AFAICT this would:

  • if the new expression is different
    • require an additional comparison vs the current code
    • do the same operations as the desired pattern (compare subexpressions, then allocate if needed)
  • if the new expression matches the old one
    • require an additional comparison vs the current code; OTOH it would avoid recursively regenerating all of the parents
    • require an additional allocation vs the desired pattern

This might not yet be ideal, but it should be a simple change that makes the code a little closer to the target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. We generally don't stress too much about compilation perf, but it would be good to avoid allocating/copying every single binary expression in the tree (which can't be simplified). What I had in mind was something the following on SqlExpressionFactory:

SqlExpression MakeBinary(ExpressionType operatorType, SqlExpression left, SqlExpression right, SqlExpression? existingBinary)

Coming from MakeBinary, existingBinary would be null so the method constructs a new SqlBinaryExpression if no simplification is possible. If, however, an existingBinary is passed and left/right can't be simplified, existingBinary is simply returned.

sqlBinaryResult.OperatorType,
sqlBinaryResult.Left,
sqlBinaryResult.Right,
sqlBinaryResult.TypeMapping,
sqlBinaryResult
)!
: result;

SqlExpression AddNullConcatenationProtection(SqlExpression argument, RelationalTypeMapping typeMapping)
Expand Down Expand Up @@ -1796,26 +1802,16 @@ private SqlExpression RewriteNullSemantics(
{
nullable = leftNullable || rightNullable;

return SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.OrElse(
body,
SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.AndAlso(leftIsNull, rightIsNull))));
return _sqlExpressionFactory.OrElse(body, _sqlExpressionFactory.AndAlso(leftIsNull, rightIsNull));
}

// doing a full null semantics rewrite - removing all nulls from truth table
nullable = false;

// (a == b && (a != null && b != null)) || (a == null && b == null)
body = SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.OrElse(
SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.AndAlso(
body,
SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.AndAlso(leftIsNotNull, rightIsNotNull)))),
SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.AndAlso(leftIsNull, rightIsNull))));
body = _sqlExpressionFactory.OrElse(
_sqlExpressionFactory.AndAlso(body, _sqlExpressionFactory.AndAlso(leftIsNotNull, rightIsNotNull)),
_sqlExpressionFactory.AndAlso(leftIsNull, rightIsNull));

if (sqlBinaryExpression.OperatorType == ExpressionType.NotEqual)
{
Expand All @@ -1826,65 +1822,6 @@ private SqlExpression RewriteNullSemantics(
return body;
}

private SqlExpression SimplifyLogicalSqlBinaryExpression(SqlExpression expression)
{
if (expression is not SqlBinaryExpression sqlBinaryExpression)
{
return expression;
}

if (sqlBinaryExpression is
{
Left: SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } leftUnary,
Right: SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual } rightUnary
}
&& leftUnary.Operand.Equals(rightUnary.Operand))
{
// a is null || a is null -> a is null
// a is not null || a is not null -> a is not null
// a is null && a is null -> a is null
// a is not null && a is not null -> a is not null
// a is null || a is not null -> true
// a is null && a is not null -> false
Comment on lines -1847 to -1848
Copy link
Contributor Author

@ranma42 ranma42 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ these two cases (the ones that return a constant) are currently not implemented in the SqlExpressionFactory methods.

It is unclear to me whether they are valuable; it would be easy to keep them (to ensure no regression), so let me know what you think

EDIT: these are now implemented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be possible to check for a stronger condition, i.e. a || !a -> true
If a is a generic expression, that would possibly involve comparing a whole subtree, so it might cause complexity issues (that could be worked around in several ways, but probably belong to a separate discussion/PR).

return leftUnary.OperatorType == rightUnary.OperatorType
? leftUnary
: _sqlExpressionFactory.Constant(
sqlBinaryExpression.OperatorType == ExpressionType.OrElse, sqlBinaryExpression.TypeMapping);
}

// true && a -> a
// true || a -> true
// false && a -> false
// false || a -> a
if (sqlBinaryExpression.Left is SqlConstantExpression { Value: bool leftBoolValue } newLeftConstant)
{
return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
? leftBoolValue
? sqlBinaryExpression.Right
: newLeftConstant
: leftBoolValue
? newLeftConstant
: sqlBinaryExpression.Right;
}

if (sqlBinaryExpression.Right is SqlConstantExpression { Value: bool rightBoolValue } newRightConstant)
{
// a && true -> a
// a || true -> true
// a && false -> false
// a || false -> a
return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
? rightBoolValue
? sqlBinaryExpression.Left
: newRightConstant
: rightBoolValue
? newRightConstant
: sqlBinaryExpression.Left;
}

return sqlBinaryExpression;
}

/// <summary>
/// Attempts to simplify a unary not operation on a non-nullable operand.
/// </summary>
Expand Down Expand Up @@ -1938,14 +1875,13 @@ protected virtual SqlExpression OptimizeNonNullableNotExpression(SqlExpression e
var left = OptimizeNonNullableNotExpression(_sqlExpressionFactory.Not(sqlBinaryOperand.Left));
var right = OptimizeNonNullableNotExpression(_sqlExpressionFactory.Not(sqlBinaryOperand.Right));

return SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.MakeBinary(
sqlBinaryOperand.OperatorType == ExpressionType.AndAlso
? ExpressionType.OrElse
: ExpressionType.AndAlso,
left,
right,
sqlBinaryOperand.TypeMapping)!);
return _sqlExpressionFactory.MakeBinary(
sqlBinaryOperand.OperatorType == ExpressionType.AndAlso
? ExpressionType.OrElse
: ExpressionType.AndAlso,
left,
right,
sqlBinaryOperand.TypeMapping)!;
}

// use equality where possible
Expand Down Expand Up @@ -2266,14 +2202,13 @@ private SqlExpression ProcessNullNotNull(SqlExpression sqlExpression, bool opera
sqlUnaryExpression.TypeMapping)!,
operandNullable);

return SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.MakeBinary(
sqlUnaryExpression.OperatorType == ExpressionType.Equal
? ExpressionType.OrElse
: ExpressionType.AndAlso,
left,
right,
sqlUnaryExpression.TypeMapping)!);
return _sqlExpressionFactory.MakeBinary(
sqlUnaryExpression.OperatorType == ExpressionType.Equal
? ExpressionType.OrElse
: ExpressionType.AndAlso,
left,
right,
sqlUnaryExpression.TypeMapping)!;
}

case SqlFunctionExpression sqlFunctionExpression:
Expand All @@ -2295,14 +2230,13 @@ private SqlExpression ProcessNullNotNull(SqlExpression sqlExpression, bool opera
sqlUnaryExpression.TypeMapping)!,
operandNullable))
.Aggregate(
(l, r) => SimplifyLogicalSqlBinaryExpression(
_sqlExpressionFactory.MakeBinary(
sqlUnaryExpression.OperatorType == ExpressionType.Equal
? ExpressionType.AndAlso
: ExpressionType.OrElse,
l,
r,
sqlUnaryExpression.TypeMapping)!));
(l, r) => _sqlExpressionFactory.MakeBinary(
sqlUnaryExpression.OperatorType == ExpressionType.Equal
? ExpressionType.AndAlso
: ExpressionType.OrElse,
l,
r,
sqlUnaryExpression.TypeMapping)!);
}

if (!sqlFunctionExpression.IsNullable)
Expand Down Expand Up @@ -2348,10 +2282,9 @@ private SqlExpression ProcessNullNotNull(SqlExpression sqlExpression, bool opera
sqlUnaryExpression.TypeMapping)!,
operandNullable))
.Aggregate(
(r, e) => SimplifyLogicalSqlBinaryExpression(
sqlUnaryExpression.OperatorType == ExpressionType.Equal
? _sqlExpressionFactory.OrElse(r, e)
: _sqlExpressionFactory.AndAlso(r, e)));
(r, e) => sqlUnaryExpression.OperatorType == ExpressionType.Equal
? _sqlExpressionFactory.OrElse(r, e)
: _sqlExpressionFactory.AndAlso(r, e));

return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7156,7 +7156,7 @@ public override async Task Query_reusing_parameter_doesnt_declare_duplicate_para
FROM (
SELECT DISTINCT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
FROM [Gears] AS [g]
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0 AND [g].[Nickname] <> @__prm_Inner_Nickname_0
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0
) AS [g0]
ORDER BY [g0].[FullName]
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public override async Task Join_composite_key(bool async)
"""
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Customers] AS [c]
INNER JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID] AND [c].[CustomerID] = [o].[CustomerID]
INNER JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
WHERE [c].[CustomerID] LIKE N'F%'
""");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5185,7 +5185,7 @@ FROM [Customers] AS [c]
WHERE (
SELECT COUNT(*)
FROM [Orders] AS [o]
WHERE [c].[CustomerID] = [o].[CustomerID] AND [c].[CustomerID] = [o].[CustomerID]) > 0
WHERE [c].[CustomerID] = [o].[CustomerID]) > 0
""");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9528,7 +9528,7 @@ UNION ALL
SELECT [o].[Nickname], [o].[SquadId], [o].[AssignedCityName], [o].[CityOfBirthName], [o].[FullName], [o].[HasSoulPatch], [o].[LeaderNickname], [o].[LeaderSquadId], [o].[Rank], N'Officer' AS [Discriminator]
FROM [Officers] AS [o]
) AS [u]
WHERE [u].[Nickname] <> @__prm_Inner_Nickname_0 AND [u].[Nickname] <> @__prm_Inner_Nickname_0
WHERE [u].[Nickname] <> @__prm_Inner_Nickname_0
) AS [u0]
ORDER BY [u0].[FullName]
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8137,7 +8137,7 @@ WHEN [o].[Nickname] IS NOT NULL THEN N'Officer'
END AS [Discriminator]
FROM [Gears] AS [g]
LEFT JOIN [Officers] AS [o] ON [g].[Nickname] = [o].[Nickname] AND [g].[SquadId] = [o].[SquadId]
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0 AND [g].[Nickname] <> @__prm_Inner_Nickname_0
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0
) AS [s]
ORDER BY [s].[FullName]
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2943,7 +2943,7 @@ public override async Task Query_reusing_parameter_doesnt_declare_duplicate_para
FROM (
SELECT DISTINCT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[PeriodEnd], [g].[PeriodStart], [g].[Rank]
FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g]
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0 AND [g].[Nickname] <> @__prm_Inner_Nickname_0
WHERE [g].[Nickname] <> @__prm_Inner_Nickname_0
) AS [g0]
ORDER BY [g0].[FullName]
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3864,7 +3864,7 @@ public override async Task Query_reusing_parameter_doesnt_declare_duplicate_para
FROM (
SELECT DISTINCT "g"."Nickname", "g"."SquadId", "g"."AssignedCityName", "g"."CityOfBirthName", "g"."Discriminator", "g"."FullName", "g"."HasSoulPatch", "g"."LeaderNickname", "g"."LeaderSquadId", "g"."Rank"
FROM "Gears" AS "g"
WHERE "g"."Nickname" <> @__prm_Inner_Nickname_0 AND "g"."Nickname" <> @__prm_Inner_Nickname_0
WHERE "g"."Nickname" <> @__prm_Inner_Nickname_0
) AS "g0"
ORDER BY "g0"."FullName"
""");
Expand Down