Simplify `NOT` by ranma42 · Pull Request #34142 · dotnet/efcore

ranma42 · 2024-07-02T20:29:13Z

When constructing Not expressions, perform some basic local simplifications.

These mostly match OptimizeNonNullableNotExpression, but they are written to to be generally applicable, even for nullable expressions.

ranma42 · 2024-07-02T20:29:37Z

This is based on top of

Simplify AND and OR #34133

and continues the same approach.

roji

This is a really, really nice improvement - I'm especially excited about how this unlocks translating via JOIN instead of OUTER/CROSS APPLY in various scenarios... A really nice demonstration of the power of early as opposed to late optimization/simplification.

See mainly nits below, other than that LGTM.

roji · 2024-07-03T10:36:36Z

+            // !(true) -> false
+            // !(false) -> true
+            // !(null) -> null
+            SqlConstantExpression sqlConstantOperand when sqlConstantOperand.Type == typeof(bool)


nit:

Suggested change

SqlConstantExpression sqlConstantOperand when sqlConstantOperand.Type == typeof(bool)

SqlConstantExpression { Value: bool boolValue, Type: var type } when type == typeof(bool)

=> Constant(!boolValue, typeof(bool), operand.TypeMapping),

(which is similar to how the check was originally in SqlNullabilityProcessor - any reason you made the change here?)

(in fact, we may want to just remove the check on the Type - if the constant expression's Value is a bool (as is pattern matched), the simplification is probably good regardless of the node's static Type.)

will this also work for null? 🤔
I guess I could handle it separately

Ah right, I see. Yeah, I'd do two checks here, one for a constant with a bool value (where the Type doesn't matter), and one for the null case (where the Type obviously does).

Ah right, I see. Yeah, I'd do two checks here, one for a constant with a bool value (where the Type doesn't matter), and one for the null case (where the Type obviously does).

Fun fact: Type is not relevant for null! 🥳
We can optimize both !null (boolean negation) and ~null (binary complement) to null, as long as we preserve the correct type and typemapping.

roji · 2024-07-03T10:37:18Z

+
+            // !(!a) -> a
+            // ~(~a) -> a (bitwise negation)
+            SqlUnaryExpression { OperatorType: ExpressionType.Not } sqlUnaryOperand => sqlUnaryOperand.Operand,


nit: I'd just use short names like unary, binary (I'm moving in that direction in general)

roji · 2024-07-03T12:35:17Z

+        };
+
+    // negate all of the results of a CaseExpression
+    private SqlExpression NotCase(CaseExpression caseExpression)


nit: if we don't inline, move to be a local function of Not()

I unified the Case methods, which made inlining this much easier

roji · 2024-07-03T12:35:45Z

+            // !(a != b) -> a == b
+            SqlBinaryExpression { OperatorType: ExpressionType.NotEqual } sqlBinaryOperand => Equal(sqlBinaryOperand.Left, sqlBinaryOperand.Right),
+
+            CaseExpression caseExpression


Add a comment to explain the cases this simplifies?

roji · 2024-07-03T12:40:08Z

+
+        return caseExpression.Operand is null
+            ? Case(clauses, newElseResult)
+            : Case(caseExpression.Operand, clauses, newElseResult);


There's some messiness in the Case() methods: the version that accepts operand accepts it as nullable SqlExpression?, but then immediately proceeds to assume it's not null with the null-forgiving operator.

We could simply allow the operand to be null, at which point it produces a CASE WHEN (just like the overload that doesn't accept an operand at all). At that point we should be able to collapse this conditional expression and inline everything into the switch above (i.e. no need for a function).

roji · 2024-07-03T12:51:03Z

+        // !(a < b) -> a >= b
+        // !(a <= b) -> a > b
+        if (sqlUnaryExpression.Operand is SqlBinaryExpression sqlBinaryOperand
+            && TryNegate(sqlBinaryOperand.OperatorType, out var negated))


Can this optimization be inside MakeBinary() like the others?

no, it is not valid in C# semantics :(
In SQL !(a < b) evaluates the same as a >= b (regardless of nulls)
In C# !(null < b) evaluates to true while null >= b evaluates to false

Makes sense, thanks!

roji · 2024-07-03T12:52:54Z

-    WHERE [l].[Id] = [l0].[Level1_Optional_Id] AND [l0].[Id] > 0
-    ORDER BY [l0].[Id]
-) AS [l1]
+LEFT JOIN (


Interesting change from OUTER APPLY to LEFT JOIN (and CROSS APPLY to INNER JOIN below) - do you have a clear idea of why this happens?

In general, any change from OUTER/CROSS APPLY to regular JOIN is a significant improvement, since OUTER/CROSS APPLY is correlated and must be calculated for each row, whereas JOIN does not. But am just interested in the exact change here.

IIUC the pipeline generates a correlationPredicate here

efcore/src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs

Lines 684 to 694 in 0337960

var correlationPredicate = ReplacingExpressionVisitor.Replace(

outerKeySelector.Parameters[0],

resultSelector.Parameters[0],

Expression.AndAlso(

ExpressionExtensions.CreateEqualsExpression(

outerKeySelector.Body,

Expression.Constant(null),

negated: true),

ExpressionExtensions.CreateEqualsExpression(

outerKeySelector.Body,

innerKeySelector.Body)));

that has the form NOT(foo == NULL) AND foo == bar, which was previously optimized to foo == bar by the SqlNullabilityProcessor.

The early optimization transforms the predicate into foo <> NULL AND foo == bar, which is simplified to foo == bar by SelectExpression.RemoveRedundantNullChecks(). I believe that this affects how the apply/join decision is made (but I have not yet delved deep in that direction).

Makes sense - and the details are indeed not super important in this context. It's great to see this unlocking optimizations like this.

This makes it easier to have unified paths in transformations.

When constructing `Not` expressions, perform some basic local simplifications. These mostly match `OptimizeNonNullableNotExpression`, but they are written to to be generally applicable, even for nullable expressions.

Most of the optimizations are eagerly performed by the `SqlExpressionFactory`. The remaining ones can now be safely applied regardless of the nullability of the expression.

ChrisJollyAU · 2024-07-25T15:01:10Z

A quick thanks for this optimization. Especially for those databases that don't support OUTER/CROSS APPLY (Microsoft Access for one), I am seeing a bunch of tests passing that previously didn't

roji · 2024-07-25T16:25:28Z

Great to hear @ChrisJollyAU! Hopefully we'll have more like these in the future as the query pipeline improves.

ranma42 force-pushed the simplify-not branch from 805902e to bea4100 Compare July 2, 2024 21:51

ranma42 marked this pull request as ready for review July 2, 2024 21:54

roji reviewed Jul 3, 2024

View reviewed changes

ranma42 added 4 commits July 3, 2024 15:53

Make operand optional in main CASE constructors

0e46a8d

This makes it easier to have unified paths in transformations.

Simplify NOT

552b99d

When constructing `Not` expressions, perform some basic local simplifications. These mostly match `OptimizeNonNullableNotExpression`, but they are written to to be generally applicable, even for nullable expressions.

Simplify NOT optimizations in SqlNullabilityProcessor

2f368ff

Most of the optimizations are eagerly performed by the `SqlExpressionFactory`. The remaining ones can now be safely applied regardless of the nullability of the expression.

Update baselines

160fe18

ranma42 force-pushed the simplify-not branch from bea4100 to 160fe18 Compare July 3, 2024 14:00

roji approved these changes Jul 3, 2024

View reviewed changes

roji merged commit 97d2365 into dotnet:main Jul 3, 2024

ranma42 deleted the simplify-not branch July 3, 2024 15:47

roji added the community-contribution label Jul 3, 2024

roji mentioned this pull request Jul 3, 2024

[9.0] New translations and SQL optimization/simplification #34151

Closed

ranma42 mentioned this pull request Aug 11, 2024

Query: Support for BETWEEN ... AND ... #12634

Open

ranma42 mentioned this pull request Jan 1, 2025

GroupJoin in EF Core 9 Returns Null for Joined Entities #35393

Closed

	SqlConstantExpression sqlConstantOperand when sqlConstantOperand.Type == typeof(bool)
	SqlConstantExpression { Value: bool boolValue, Type: var type } when type == typeof(bool)
	=> Constant(!boolValue, typeof(bool), operand.TypeMapping),

	var correlationPredicate = ReplacingExpressionVisitor.Replace(
	outerKeySelector.Parameters[0],
	resultSelector.Parameters[0],
	Expression.AndAlso(
	ExpressionExtensions.CreateEqualsExpression(
	outerKeySelector.Body,
	Expression.Constant(null),
	negated: true),
	ExpressionExtensions.CreateEqualsExpression(
	outerKeySelector.Body,
	innerKeySelector.Body)));

Conversation

ranma42 commented Jul 2, 2024

Uh oh!

ranma42 commented Jul 2, 2024

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranma42 Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisJollyAU commented Jul 25, 2024

Uh oh!

roji commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ranma42 Jul 3, 2024 •

edited

Loading