Relational: Make table reference graph mapping consistent #24701
Conversation
|
Hello @smitpatel! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
| @@ -393,7 +393,8 @@ public void ApplyDistinct() | |||
| /// <summary> | |||
| /// Adds expressions from projection mapping to projection if not done already. | |||
| /// </summary> | |||
| public Expression ApplyProjection(Expression shaperExpression, QuerySplittingBehavior querySplittingBehavior) | |||
| public Expression ApplyProjection( | |||
There was a problem hiding this comment.
We already have broken this API in this release. Need resultCardinality for split collections.
| @@ -3061,6 +3062,18 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) | |||
| Offset = (SqlExpression?)visitor.Visit(Offset); | |||
| Limit = (SqlExpression?)visitor.Visit(Limit); | |||
|
|
|||
| //var identifier = VisitList(_identifier.Select(e => e.Column).ToList(), inPlace: true, out _) | |||
There was a problem hiding this comment.
These are needed for split collection work, but decided to submit that work in separate PR so just left this here commented for now.
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
| @@ -584,5 +604,125 @@ public ClientProjectionRemappingExpressionVisitor(object[] clientProjectionIndex | |||
| return base.Visit(expression); | |||
| } | |||
| } | |||
|
|
|||
| private sealed class SelectExpressionVerifyingExpressionVisitor : ExpressionVisitor | |||
There was a problem hiding this comment.
this visitor is not used anywhere - purely a debugging tool? (i.e. we don't want to run always in debug, like we do with table aliases?)
There was a problem hiding this comment.
Yes, it is for debugging. Split query introduces clone method on select expression which was causing a lot of errors (for obvious reason), so I added this. I will see if there is a place where I can add this. It is somewhat more complicated than table aliases as this is more stricter form of graph consistency, so it cannot be run during most point when we are mutating SelectExpression.
ghost
merged commit 
Also add a visitor to verify that all table references are proper
Update alias unique-fier to skip already visited expression
Fixes #24473
Fixes #24460