Redo SQL table alias management #32785
Merged
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
commented
Jan 11, 2024
| /// <returns>An expression representing a SELECT in a SQL tree.</returns> | ||
| SelectExpression Select(SqlExpression? projection); | ||
| [EntityFrameworkInternal] | ||
| SelectExpression Select(SqlExpression? projection, SqlAliasManager sqlAliasManager); |
There was a problem hiding this comment.
In the longer-term plan, these are temporary - Select shouldn't be constructing tables within itself as it's doing now, but those tables should rather be constructed outside, i.e. in RelationalQueryableMethodTranslatingEV.
| query = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory, _useRelationalNulls) | ||
| .Visit(query); | ||
| query = new RelationalValueConverterCompensatingExpressionVisitor(RelationalDependencies.SqlExpressionFactory).Visit(query); | ||
| var query5 = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory, _useRelationalNulls) |
There was a problem hiding this comment.
Changed to make it easier to debug and compare multiple versions of the tree in the various phases.
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| [EntityFrameworkInternal] | ||
| public void ChangeTableAlias(int tableIndex, string newAlias, HashSet<TableReferenceExpression> visitedTableReferences) |
There was a problem hiding this comment.
This is a very temporary hack to allow SqlAliasManager to change the alias during post-processing. The subsequent PR will remove TableReferenceExpression altogether and this will go away.
| new SelectExpressionMutableVerifyingExpressionVisitor().Visit(query); | ||
| // Verifies that all table aliases are uniquely assigned without skipping over | ||
| // Which points to possible mutation of a SelectExpression being used in multiple places. | ||
| new TableAliasVerifyingExpressionVisitor().Visit(query); |
There was a problem hiding this comment.
This verification visitor no longer makes sense, but I'll be introducing something better to check correct table/column scoping, after TableReferenceExpression is gone.
roji
force-pushed
the
TableAliases
branch
2 times, most recently
from
a7c0ab5
to
ec44934
Compare
maumar
reviewed
Jan 15, 2024
test/EFCore.SqlServer.FunctionalTests/Query/JsonQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
maumar
reviewed
Jan 15, 2024
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
maumar
reviewed
Jan 15, 2024
src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs
Outdated
Show resolved
Hide resolved
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
maumar
reviewed
Jan 15, 2024
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
maumar
approved these changes
Jan 15, 2024
Closees dotnet#32784 Fixes dotnet#32757 Part of dotnet#32778 Fixes dotnet#26104
maumar
approved these changes
Jan 15, 2024
ranma42
added a commit
to ranma42/efcore
that referenced
this pull request
May 24, 2024
They were fixed by dotnet#32785.
cincuranet
pushed a commit
that referenced
this pull request
May 24, 2024
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.
Note to reviewers: since we now generate unique aliases early - before constructing the table - and not later in a separate visitation, the alias ordering is changing (deeper nested tables have lower numbers); that's why so many SQL baselines are changing in this PR.
Closes #32784
Fixes #32757
Part of #32778
Fixes #26104