search

dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.66k stars 3.15k forks source link

Apply early SqlExpression optimizations (e.g. x AND true -> x) in VisitChildren #34556

Open roji opened 3 weeks ago

roji commented 3 weeks ago

We have been starting to apply certain optimizations early, at creation in SqlExpressionFactory; for example, if someone calls SqlExpressionFactory.MakeBinary with x AND true, x is returned.

We should do the same optimizations in e.g. SqlBinaryExpression.VisitChildren(), and possibly even in SqlBinaryExpression.Update(). This will ensure that arbitrary visitors that happen to simplify an operand of a binary expression would transparently get the whole binary expression optimized away.

Ideally, we'd factor things in a way that the same simplification code is used from both SqlExpressionFactory and from SqlBinaryExpression itself (probably have the code on SqlBinaryExpression and call it from SqlExpressionFactory).

(note that this applies to other expression types, not just SqlBinaryExpression)

/cc @ranma42

ranma42 commented 3 weeks ago

Am I correct in the assumption that VisitChildren and Update do not need to recompute the type mapping?

roji commented 3 weeks ago

@ranma42 they certainly don't today. The moment some visitor needs to do something fancy and do a custom change to any node type (e.g. change the type mapping), it's their responsibility to handle that node type themselves; VisitChildren is only the generic visitation logic for visitors that don't have such custom logic.

But I might be missing your meaning... What scenario do you have in mind, or possible concern?

ranma42 commented 3 weeks ago

If that assumption holds true, it should be fairly straightforward to split the "simple optimizations" (constant folding & similar) out of the SqlExpressionFactory and still get useable expressions. Basically my idea is to move those optimizations into static methods (either in SqlExpressionFactory or maybe a SqlExpressionSimplifier, ...) and instead leave the ApplyTypeMapping part as responsibility of SqlExpressionFactory (whose instance methods can rely on the dependencies which are specific to the provider).

roji commented 3 weeks ago

Yep, sounds right. I'd consider just putting the static methods on each expression type (SqlBinaryExpression would hold the method that knows how to simplify itself).

BTW just noting that we can't do this until efficient deep comparison is implemented (#34149), since this means that all visitors will start doing comparisons for all binary expressions (and other) once they're changed by any visitor anywhere.