Redo query column/table relationship without TableReferenceExpression #32812

roji · 2024-01-14T10:55:07Z

~~NOTE: THIS IS BASED ON TOP OF #32785 (ALIAS REDO), REVIEW ONLY THE 2ND COMMIT~~

After this, TableReferenceExpression no longer exists, and ColumnExpressions refer to their tables by their string aliases.
Lots of table reference book-keeping/maintenance code has been removed, including various visitation passes (compilation performance).
There aren't many places where a visitor actually needs to resolve a column to its table; in most cases these are on the current/top-most SelectExpression, which is trivial. In cases where columns can refer up the SQL tree, the visitor tracks the mapping as part of its visitation (this is very rare).
ColumnExpression is now a regular, publicly-constructible expression - there's no more private ConcreteColumnExpression.
Note that after this PR, table aliases are still mutable - a small upcoming PR will finish that bit off (in the interest of not cramming too much into a single PR).

roji · 2024-01-14T11:39:56Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

-                {
-                    table = joinExpressionBase.Table;
-                }
-                else if (table is SetOperationBase setOperationBase)


@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.

roji · 2024-01-14T11:40:34Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -2139,6 +2144,11 @@ public IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping?

        protected override Expression VisitExtension(Expression node)
        {
+            if (node is TableExpressionBase { Alias: string tableAlias } table)


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.

roji · 2024-01-14T11:41:31Z

src/EFCore.Relational/Query/SqlAliasManager.cs

+                        // 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;


Note that TableExpressionBase.Alias is still mutable. It will be made immutable in a separate PR.

roji · 2024-01-14T11:42:16Z

src/EFCore.Relational/Query/SqlExpressions/ColumnExpression.cs

@@ -13,55 +13,85 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 ///     </para>
 /// </summary>
 [DebuggerDisplay("{TableAlias}.{Name}")]
-public abstract class ColumnExpression : SqlExpression
+public class ColumnExpression : SqlExpression


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.

roji · 2024-01-14T11:47:40Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

    {
        if (_mutable)
        {
+            VisitList(_tables, inPlace: true, out _);


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.

roji · 2024-01-14T11:48:02Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

-                // 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)


All this table reference updating/cloning is now gone 🎉

roji · 2024-01-14T11:48:57Z

src/EFCore.Relational/Query/SqlTreePruner.cs

+    /// </remarks>
+    // TODO: Make this protected after SelectExpression.Prune is moved into this visitor
+    [EntityFrameworkInternal]
+    public virtual string? CurrentTableAlias { get; set; }


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.

maumar · 2024-01-17T16:47:44Z

src/EFCore.Relational/Query/SqlExpressions/TableExpressionBase.cs

+    /// </summary>
+    [EntityFrameworkInternal]
+    public virtual string GetRequiredAlias()
+        => Alias ?? throw new InvalidOperationException($"No alias is defined on table: {ExpressionPrinter.Print(this)}");


add resource string

Oops missed this one, will do in the #32815

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

Closes dotnet#32558

roji requested a review from a team January 14, 2024 10:55

roji commented Jan 14, 2024

View reviewed changes

This was referenced Jan 14, 2024

Make TableExpressionBase.Alias immutable #32815

Merged

[9.0] Query pipeline architecture improvements #31327

Closed

roji force-pushed the TableReferenceBegone branch from b0607d0 to 767a4f2 Compare January 16, 2024 22:12

maumar reviewed Jan 17, 2024

View reviewed changes

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved

AndriySvyryd approved these changes Jan 18, 2024

View reviewed changes

maumar approved these changes Jan 18, 2024

View reviewed changes

roji force-pushed the TableReferenceBegone branch from 767a4f2 to 37d9118 Compare January 18, 2024 07:11

Redo query column/table relationship without TableReferenceExpression

ac176a4

Closes dotnet#32558

roji force-pushed the TableReferenceBegone branch from 37d9118 to ac176a4 Compare January 18, 2024 07:13

roji mentioned this pull request Jan 18, 2024

Redo SQL table alias management #32785

Merged

roji merged commit 1549daa into dotnet:main Jan 18, 2024
7 checks passed

roji deleted the TableReferenceBegone branch January 18, 2024 07:56

This was referenced Jan 22, 2024

Consider breaking out SelectExpression internal logic to the translation visitors #31049

Open

Invalid column name 'key' in SQL Server JSON query #32910

Closed

Make our SQL tree fully immutable #32927

Open

roji mentioned this pull request Jan 31, 2024

Stop referencing the query from ProjectionBindingExpression in the shaper #32980

Open

roji mentioned this pull request Jun 10, 2024

Table alias regression on ExecuteUpdate on SQL Server #33937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo query column/table relationship without TableReferenceExpression #32812

Redo query column/table relationship without TableReferenceExpression #32812

roji commented Jan 14, 2024 •

edited

Loading

roji Jan 14, 2024

roji Jan 14, 2024

roji Jan 14, 2024

roji Jan 14, 2024

roji Jan 14, 2024

roji Jan 14, 2024

roji Jan 14, 2024

maumar Jan 17, 2024

roji Jan 18, 2024

Redo query column/table relationship without TableReferenceExpression #32812

Redo query column/table relationship without TableReferenceExpression #32812

Conversation

roji commented Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Jan 14, 2024 •

edited

Loading