Move all pruning code to the pruner and make it an immutable visitor #32817
Conversation
| tables.Add(newTable); | ||
| } | ||
|
|
||
| var projections = this.VisitAndConvert(selectExpression.Projection); |
There was a problem hiding this comment.
This both switches to using VisitAndConvert (which visits lists and returns the original if it didn't change), and removes tracking of whether anything changed; SelectExpression.Update() does that now - as is standard with expressions - and returns the original SelectExpression if nothing changed.
| @@ -197,7 +197,7 @@ protected override Expression VisitDelete(DeleteExpression deleteExpression) | |||
| && selectExpression.Orderings.Count == 0 | |||
| && selectExpression.GroupBy.Count == 0 | |||
| && selectExpression.Tables.Count == 1 | |||
| && selectExpression.Tables[0] == deleteExpression.Table | |||
| && selectExpression.Tables[0].Equals(deleteExpression.Table) | |||
There was a problem hiding this comment.
Note places like this where reference identity was assumed; but if the tables were replaced for some reason (e.g. alias rewriting), they now get duplicated (immutable visitors); structural equality is used instead.
| @@ -64,20 +64,22 @@ public override ExpressionType NodeType | |||
| protected override Expression VisitChildren(ExpressionVisitor visitor) | |||
| { | |||
| var selectExpression = (SelectExpression)visitor.Visit(SelectExpression); | |||
| var table = (TableExpression)visitor.Visit(Table); | |||
There was a problem hiding this comment.
We had a bug where the TableExpression on Delete/UpdateExpression wasn't being visited (this apparently wasn't necessary until now).
| throw new InvalidOperationException(RelationalStrings.SelectExpressionUpdateNotSupportedWhileMutable); | ||
| } | ||
|
|
||
| if (projections == Projection |
| * SelectExpression can appear at 2 levels, | ||
| * 1. Top most level which is always same reference when translation phase, post-translation it can change in | ||
| * ShapedQueryExpression where it would cause reconstruction. Reconstruction is cheaper than computing whole Equals | ||
| * 2. Nested level component inside top level SelectExpression where it could change the reference and reconstruct SQL tree. |
There was a problem hiding this comment.
It's worth understanding and thinking about this a bit - the mutable/immutable/equality situation was very squishy up until now.
First, this comment isn't entirely accurate; we sometimes have the same expression node referenced from multiple places in the tree; if non-mutating visitor visits that, the single node gets visited twice and duplicated; and if this happens in post-processing, that could produce two nodes with the same alias. Note also that for almost all other SQL expression nodes, Equals really does implement structural equality - only SelectExpression (and TableExpression) were an exception.
Since this PR turns the pruner into a non-mutating visitor, this now happens more often and exposes issues. For example, SqlNullabilityProcessor had logic which did Equals on two nodes; these happened to be previously the same instance, but pruning can now make them two structurally equal instances instead.
Whether/when we should performing potentially expensive recursive equality - i.e. is it worth it for the aforementioned null semantics optimization - is another question... Similarly, performing a fully recursive hash code calculation isn't required and is probably needlessly expensive (see #19859 and #19860).
NOTE: This is based on top of #32815, review the last commit only
Fixes #31276