Redo query column/table relationship without TableReferenceExpression #32812
Redo query column/table relationship without TableReferenceExpression #32812roji merged 1 commit intodotnet:mainfrom
Conversation
| { | ||
| table = joinExpressionBase.Table; | ||
| } | ||
| else if (table is SetOperationBase setOperationBase) |
There was a problem hiding this comment.
@maumar unless I'm mistaken, there was a bug here when a set operation was wrapped in a join; we'd unwrap the join above but not go into the set operation because of the "else" here.
|
|
||
| protected override Expression VisitExtension(Expression node) | ||
| { | ||
| if (node is TableExpressionBase { Alias: string tableAlias } table) |
There was a problem hiding this comment.
This is one of the few examples of a visitor which really does need to go from columns to tables, and so maintains a table map during visitation.
| // TODO: recreating a new TableExpressionBase implementation with a different alias. | ||
| // TODO: Either add a mandatory, abstract non-destructive ChangeAlias to TableExpressionBase, or ideally, | ||
| // TODO: refactor things to split the alias out of TableExpressionBase altogether. | ||
| table.Alias = newAlias; |
There was a problem hiding this comment.
Note that TableExpressionBase.Alias is still mutable. It will be made immutable in a separate PR.
| /// </summary> | ||
| [DebuggerDisplay("{TableAlias}.{Name}")] | ||
| public abstract class ColumnExpression : SqlExpression | ||
| public class ColumnExpression : SqlExpression |
There was a problem hiding this comment.
Note that ColumnExpression is no longer abstract, with a private ConcreteColumnExpression that only SelectExpression can construct. This is now a plain, regular SqlExpression that anyone can build.
| { | ||
| if (_mutable) | ||
| { | ||
| VisitList(_tables, inPlace: true, out _); |
There was a problem hiding this comment.
Note that table visitation has been moved to be the first. This aligns with actual evaluation order of SQL SELECT - tables are evaluated first, and projections last - they can reference tables (the textual layout of SQL SELECT has always seemed like a mistake; IDEs can't to Intellisense as the user is writing projections since the tables haven't been written yet... Note that in C# LINQ expression syntax the tables come first).
This is a more useful for visitors which need to visit tables first, for tracking purposes etc.
| // Otherwise, if no tables have changed, we mutate the TableReferenceExpressions (this was the previous code, left it for | ||
| // a more low-risk fix). Note that updateColumns is false only if we're already being called from | ||
| // ColumnTableReferenceUpdater to replace the ColumnExpressions, in which case we avoid infinite recursion. | ||
| if (tablesChanged && updateColumns) |
There was a problem hiding this comment.
All this table reference updating/cloning is now gone 🎉
| /// </remarks> | ||
| // TODO: Make this protected after SelectExpression.Prune is moved into this visitor | ||
| [EntityFrameworkInternal] | ||
| public virtual string? CurrentTableAlias { get; set; } |
There was a problem hiding this comment.
When everything is done, I plan to do a final cleanup here to move the SelectExpression.Prune functionailty into this visitor; at that point everything will be nice and self-contained. Right now it's somewhat smelly - that's a temporary situation.
roji
force-pushed
the
TableReferenceBegone
branch
from
b0607d0 to
767a4f2
Compare
| /// </summary> | ||
| [EntityFrameworkInternal] | ||
| public virtual string GetRequiredAlias() | ||
| => Alias ?? throw new InvalidOperationException($"No alias is defined on table: {ExpressionPrinter.Print(this)}"); |
roji
force-pushed
the
TableReferenceBegone
branch
from
767a4f2 to
37d9118
Compare
roji
force-pushed
the
TableReferenceBegone
branch
from
37d9118 to
ac176a4
Compare
3 participants
NOTE: THIS IS BASED ON TOP OF #32785 (ALIAS REDO), REVIEW ONLY THE 2ND COMMITCloses #32558