Efcore: Query: Assign aliases to tables differently

[smitpatel]

A selectExpression contains projections & other constructs, which refers to tables in that select expression. When visiting children of select expression if that table is updated then it causes re-generation for each node using it. Hence the constraint is violated. While it has low functional impact but it means that we need to do deep equals every time we want to compare. Ideally, VisitChildren should take care of re-connecting nodes.

[roji]

This also has an important perf aspect, since we're visiting the same TableExpressions via both projections and tables. #17455 shows an extreme, exponential effect where only 7 includes cause over 20 seconds of query compilation time.

Clearing milestone to consider for 3.1.

[NetTecture]

"Clearing milestone to consider for 3.1." - unless you plan to pass 3.1 within 4 weeks of 3.0 or so that means for me and a lot of people like me EFCore is totally absolutely UNUSABLE because we DO have 10+ includes in large parts of our application and if you go from "instant compilation" to "minutes" that is a regression that is NOT ready even for development.

[roji]

Any objection to me poaching this @smitpatel?

[smitpatel]

Yes. I will fix this.

[smitpatel]

@roji - If you have ideas around it and want to work on this, then please schedule a meeting and bring a design. I want to avoid you doing work and then having to discard because of lack of communication around complexity and areas this would touch.

[roji]

At least as a first step, the idea was simply to collect old to new table mappings as we visit SelectExpression.Tables, and then use ReplacingExpressionVisitor on each query component before visiting it. You've already removed visitation in ColumnExpression in #17651, so unless we have other forms of references to tables from within query constructs, it seems like it should take care of it. Am I missing other aspects?

[smitpatel]

How would you do it if Visitation of SelectExpression is outside of your control? i.e. what if SelectExpression.VisitChildren is not used?

[roji]

At least for 3.1, where we're still doing low-risk bugfixes only, it may be enough to identify current visitors which visit SelectExpression themselves and apply the same logic (e.g. SearchConditionConvertingExpressionVisitor, not sure if there are others). This is similar to what you've done in #17651 - we no longer visit tables via ColumnExpressions but badly-written visitors may still do that.

A more extreme approach would be to factor out the various visitations from VisitChildren (e.g. VisitProjections, VisitPredicate), and not allow SqlExpressionVisitor subclasses to override VisitSelect, locking down the correct behavior. Since we have very little visitors at the moment, it's difficult to know whether that would prevent useful/legitimate overriding scenarios, so it doesn't seem right (or at least premature). Even if we do this, it's always possible for a visitor to extend ExpressionVisitor directly and mess up a SelectExpression in various ways.

Finally, we can add a verification (debug-only?) visitor at end of postprocessing to verify that referential integrity is maintained, to catch any bugs (like SqlTypeMappingVerifyingExpressionVisitor for missing type mappings).

But if I'm missing something or if you have something different in mind for 3.1 we can just have a conversation about it.

[smitpatel]

None of those fixes the crux of the issue. While they resolve things for internal visitors, they still leave scope for external ones to do things incorrectly. If that is what we are heading at (fixing for internal visitors), then the solution we have in codebase right now is good enough.

There are altogether different ideas how to deal with this so that this problem does not arise in first place. Hence my hesitance over this issue being poachable.

[roji]

If that is what we are heading at (fixing for internal visitors), then the solution we have in codebase right now is good enough.

Unless I'm mistaken, #17651 doesn't fix the referential integrity issue in any way - only the perf issue (by skipping visitation in ColumnExpression) - so in the current codebase, ColumnExpressions currently point to potentially old tables which may have been replaced by visitors. It seems to me that this is well worth fixing in 3.1 regardless of external visitors, but I may be mistaken (i.e. there may not be an actual problem caused by this at the moment).

An airtight solution with regards to external visitors would lock down access to SelectExpression.Tables, so that if a visitor wants to replace a table they'd have through a special method on SelectExpression (e.g. ReplaceTable), which would also perform the replacement in all query components. Depending on how far we want to go, we may also want to prevent the possibility of a visitor to replace a ColumnExpression with another that refers to another table (TableExpression and SelectExpression have internal-only constructors, but not set operations). I assumed such design changes would be a too much for 3.1, but again maybe you see things differently.

I'm fine discussing what to do, or if you prefer I can leave this issue to you.

[ajcvickers]

Note from triage: This issue is the root cause for #19763, which is blocking OData customers. (Do not punt from 6.0.)

[smitpatel]

Design meeting notes:

There are 2 places in our query expression tree which violates the tree principal.
ShapedQueryExpression - The shaper needs to know the query expression. Since we combine shapers from multiple queries when translating a subquery in projection. We achieve this by using ProjectionBindingExpression which remembers the query expression. Because of this (and also legacy) reasons we always updates query expression in place and it does not change during translation. Side effects of this -> Pushdown into subquery is in place. Our query expressions are mutable. We need this weak link due to multiple subqueries and we cannot update it because places where we need to pushdown are not directly connected to shaper. We may just leave with this for now.

SelectExpression - The columns references the tables and tables are part of SelectExpression.Tables.
We require a link here since tables can change their aliases. In past we tried to assign unique alias at creation time. It had some issues. There are multiple different places where aliases are created. We used QCC which is central location for a query during compilation for aliasing. But few of the places are also inside SelectExpression itself which added a service inside SelectExpression causing a memory leak. So assigning unique alias through QCC is not possible. Same issue with ISqlExpressionFactory as a central location. Further, without unification, we cannot have proper aliases because there could be different subqueries using table same tables needing different aliases if they are joined together. Our current approach involves that we make aliases unique after translation is done and no more tables are added. Hence the column referencing the table makes sure that the it uses correct alias for table. Issue arises since this link is not weak. We store actual tableexpression in column. During optimization we optimize the tables in SelectExpression (happens when they are nested SelectExpression). We ended up visiting same subquery N+1 times where N is the time there is column coming out of the subquery. To avoid bad perf, what we did was to skip visiting table from column expression in 3.0. Essentially breaking referential integrity. Which had few side-effects of generating invalid SQL in some cases. To resolve out, we are moving towards same weak link approach we are using above and utilizing the fact that SelectExpression is mutable in the process.

Proposal:
We will create a TableReferenceExpression which holds the SelectExpression + alias of the table. ColumnExpression will hold TableReferenceExpression so visitation does not go inside. TableReferenceExpression.Alias would be a mutable property so we can make aliases unique. The way to get the main table would be to look into SelectExpression and find the table with matching alias (so we can always reference actual table if we need). TableReferenceExpression will be created by SelectExpression only when adding table and integrate with generation of column expressions so there are no invalid columns. Table aliases are updated correctly and referential integrity is maintained.

• This will also make that SelectExpression is fully mutable (rather than us trying to not mutate it after translation). We can revisit this in future if we come across a need where we need to make it immutable.
• Uniqufication can be done post-translation or during translation inside select expression. It is somewhat orthogonal to this proposal but we should do it during translation for ease of understanding the query.
• PushdownInSubquery will need to update all the nested column expressions since the SelectExpression will move inside subquery. For all other cases we lift projections from subquery into outer which should generate correct table reference automatically.