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.