ExecuteUpdate: Allow using other tables in the query to generate result set #28731
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 (
|
|
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 [email protected] 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. |
smitpatel
force-pushed
the
smit/executeupdate2
branch
from
26d4fe5 to
f1f2086
Compare
|
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 [email protected] 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. |
|
|
||
| Visit(table); | ||
|
|
||
| void LiftPredicate(TableExpressionBase joinTable) |
There was a problem hiding this comment.
For this kind of SQL manipulation, remember I moved the logic for PG DELETE to a separate visitor at the end of the pipeline, to keep SQL generation simple/clean. We could do something similar here, though of course it's not strictly necessary (it's just a pretty unique case where we're doing that in SQL generation).
I'll probably just do use the same visitor for update in PG 😈
There was a problem hiding this comment.
I considered it but there were few differences in that case. Using doesn't allow using any joins so you would need to move everything. Here it is just 1 table which needs to changed that way so I just inlined here. Also this is relational which SqlServer overrides, changing expression here means providers other than Sqliite/Postgre need to block the conversion. Which makes task more overall.
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// By default, only single-table select expressions are supported, and optionally with a predicate. | ||
| /// By default, only muli-table select expressions are supported, and optionally with a predicate. |
There was a problem hiding this comment.
| /// By default, only muli-table select expressions are supported, and optionally with a predicate. | |
| /// By default, single- or multi-table select expressions are supported, optionally with a predicate. |
There was a problem hiding this comment.
I will make this change in next PR to get this PR to pass and merge. Not confident that if I push another commit here, the mighty build infra will make it pass before deadline.
| && selectExpression.Projection.Count == 0) | ||
| { | ||
| Sql.Append("UPDATE "); | ||
| GenerateTop(selectExpression); |
There was a problem hiding this comment.
This is so weird... especially since it doesn't respect any ordering... So what, change only 10 random rows and leave the others??
There was a problem hiding this comment.
I want you to change destiny of people but only x number of them. Rest can suffer their entire life!
There was a problem hiding this comment.
Wait, actually.... just to confirm...
If the user does OrderBy and then Take, will it translate correctly? I mean, if you push both operators in the the subquery it's fine, but if the TOP is at the top-level and the OrderBy is in the query, then the translation is bad...
There was a problem hiding this comment.
(removing auto-merge just to be on the safe side)
There was a problem hiding this comment.
existing order by causes subquery and top goes inside the subquery.
|
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 [email protected] 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 [email protected] 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. |
ghost
merged commit 
ajcvickers
changed the title
Part of #795