Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve pruning to properly support ExecuteUpdate #31407

Closed
roji opened this issue Aug 3, 2023 · 0 comments · Fixed by #32672
Closed

Improve pruning to properly support ExecuteUpdate #31407

roji opened this issue Aug 3, 2023 · 0 comments · Fixed by #32672
Assignees
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Aug 3, 2023

#31406, allowed ExecuteUpdate with entity splitting (as long as all updated columns are on the same table). While this works well, if you try to change columns on a non-main (split) table on SQL Server, this fails (see test NonSharedModelBulkUpdatesTestBase.Update_single_table_in_entity_with_entity_splitting); - this does work on SQLite, due to a different query shape.

The root cause here is that we generate an INNER JOIN between the main table and the split table, where the split table is the one being updated. When we prune the expression after translation (SelectExpressionPruningExpressionVisitor), we don't find any reference to the joined table within the SelectExpression: the only references are from the UpdateExpression's column setters, but the pruning visitor doesn't see them.

The proper fix here would be to make pruning a proper visitor, with specific handling for ExecuteUpdate. More generally, we should consider not having UpdateExpression be a wrapper around SelectExpression, but its own expression type that directly contains its elements. This may involve significant work as all postprocessing visitors would need to handle it properly, but it's the more correct representation and would prevent bugs like this from the get-go.

See also:

  • #31083, which is related (pruning also doesn't visit all SelectExpressions since it isn't a real visitor).
  • #31049, which is about extracting the visitation out of SelectExpression so that providers can extend for provider-specific expressions (such as OPENJSON).
@roji roji self-assigned this Aug 3, 2023
@ajcvickers ajcvickers added this to the Backlog milestone Sep 22, 2023
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
@roji roji modified the milestones: Backlog, 9.0.0 Dec 25, 2023
@roji roji added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed consider-for-current-release labels Dec 25, 2023
roji added a commit to roji/efcore that referenced this issue Dec 25, 2023
roji added a commit that referenced this issue Dec 29, 2023
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
@roji roji modified the milestones: 9.0.0-preview1, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants