ExecuteUpdate: Allow using other tables in the query to generate result set #28731

smitpatel · 2022-08-15T19:31:25Z

Part of #795

ghost · 2022-08-15T19:31:44Z

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-08-15T19:47:45Z

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:

Azure Pipelines

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 this

From the bot dev team

We'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.

…lt set Part of #795

ghost · 2022-08-15T21:05:43Z

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:

Azure Pipelines

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 this

From the bot dev team

We'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.

roji · 2022-08-15T21:18:17Z

src/EFCore.Relational/Query/QuerySqlGenerator.cs

+
+                    Visit(table);
+
+                    void LiftPredicate(TableExpressionBase joinTable)


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 😈

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.

roji · 2022-08-15T21:20:43Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

    /// </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.


Suggested change

/// 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.

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.

roji · 2022-08-15T21:24:39Z

src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs

            && selectExpression.Projection.Count == 0)
        {
            Sql.Append("UPDATE ");
+            GenerateTop(selectExpression);


This is so weird... especially since it doesn't respect any ordering... So what, change only 10 random rows and leave the others??

I want you to change destiny of people but only x number of them. Rest can suffer their entire life!

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...

(removing auto-merge just to be on the safe side)

existing order by causes subquery and top goes inside the subquery.

Perfect 👍

ghost · 2022-08-15T21:26:36Z

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:

Azure Pipelines

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 this

From the bot dev team

We'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.

ghost · 2022-08-15T21:51:59Z

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:

Azure Pipelines

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 this

From the bot dev team

We'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.

smitpatel requested a review from a team August 15, 2022 19:31

smitpatel added the auto-merge label Aug 15, 2022

ExecuteUpdate: Allow using other tables in the query to generate resu…

f1f2086

…lt set Part of #795

smitpatel force-pushed the smit/executeupdate2 branch from 26d4fe5 to f1f2086 Compare August 15, 2022 20:49

roji approved these changes Aug 15, 2022

View reviewed changes

roji removed the auto-merge label Aug 15, 2022

smitpatel added the auto-merge label Aug 15, 2022

ghost merged commit 5c5ccfa into main Aug 15, 2022

ghost deleted the smit/executeupdate2 branch August 15, 2022 22:14

ajcvickers changed the title ~~ExecuteUpdate: Allow using other tables in the query to generate resu…~~ ExecuteUpdate: Allow using other tables in the query to generate result set Aug 18, 2022

ajcvickers mentioned this pull request Aug 18, 2022

.NET Data Biweekly Updates (2022) #27185

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExecuteUpdate: Allow using other tables in the query to generate result set #28731

ExecuteUpdate: Allow using other tables in the query to generate result set #28731

smitpatel commented Aug 15, 2022 •

edited by ajcvickers

Loading

ghost commented Aug 15, 2022

ghost commented Aug 15, 2022

From the bot dev team

ghost commented Aug 15, 2022

From the bot dev team

roji Aug 15, 2022

smitpatel Aug 15, 2022

roji Aug 15, 2022

smitpatel Aug 15, 2022

roji Aug 15, 2022

smitpatel Aug 15, 2022

roji Aug 15, 2022

roji Aug 15, 2022

smitpatel Aug 15, 2022

roji Aug 15, 2022

ghost commented Aug 15, 2022

From the bot dev team

ghost commented Aug 15, 2022

From the bot dev team


		Visit(table);

		void LiftPredicate(TableExpressionBase joinTable)

	/// 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.

ExecuteUpdate: Allow using other tables in the query to generate result set #28731

ExecuteUpdate: Allow using other tables in the query to generate result set #28731

Conversation

smitpatel commented Aug 15, 2022 • edited by ajcvickers Loading

ghost commented Aug 15, 2022

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 15, 2022

From the bot dev team

ghost commented Aug 15, 2022

From the bot dev team

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 15, 2022

From the bot dev team

ghost commented Aug 15, 2022

From the bot dev team

smitpatel commented Aug 15, 2022 •

edited by ajcvickers

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.