Fix expression cloning when table changes in SelectExpression.VisitChildren #32456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roji
force-pushed
the
DuplicateSelect
branch
2 times, most recently
from
2ac3090 to
cee4b79
Compare
maumar
reviewed
Nov 30, 2023
maumar
approved these changes
Dec 1, 2023
roji
added a commit
to roji/efcore
that referenced
this pull request
Dec 4, 2023
…ildren (dotnet#32456) Fixes dotnet#32234 (cherry picked from commit cf5ec40)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a table is changed in SelectExpression.VisitChildren() (on an immutable SelectExpression), visit the entire SelectExpression and replace ColumnExpressions to have them properly point to the new SelectExpression, without sharing TableReferenceExpression. This allows us to mutate the TableReferenceExpression later (i.e. alias uniquification) without affecting two queries in the tree.
There's a bit of added complexity here to prevent infinite recursion where the new ColumnTableReferenceUpdater that we call also uses SelectExpression.VisitChildren, but we don't want/need that to go into ColumnTableReferenceUpdater again.
This should be a temporary fix; I believe we should revisit the general design around tables, ColumnExpression and TableReferenceExpression.
Finally, this isn't a super trivial fix - would appreciate a deep review (especially as we may consider this for patching).
Fixes #32234
Note that I ran into #26104 as well while testing this, which is a debug-only issue that's very related to this one (same SelectExpression instance referenced from multiple parts of the query).