fix(core): reject a DELETE or an UPDATE whose WHERE clause cannot reach the provider - #24657
Open
michaelsembwever wants to merge 1 commit into
Open
Conversation
…ch the provider A `DELETE` or an `UPDATE` whose `WHERE` clause holds an `IN` or an `EXISTS` subquery changed every row of the target table, and reported the whole table as affected. The optimizer rewrites the subquery into a semi join, so the condition leaves the `Filter` nodes that `extract_dml_filters()` reads. The provider then received an empty filter list, which is the encoding for "no WHERE clause", and applied the statement to all rows. An always-false `WHERE` clause reached the provider the same way. The simplifier folds the predicate into an empty relation, so again no filter survived, and a `DELETE FROM t WHERE false` emptied the table. Add `classify_dml_input()`, which walks the input plan of a `DELETE` or an `UPDATE` before the provider hook runs: - an empty relation means that no row matches, so the statement reports a count of 0 and the hook is not called; - a join, a predicate on another table, or any other node that restricts or multiplies rows raises a "not implemented" error, and the hook is not called. The hook stays untouched in every rejected case, so a provider that writes to durable storage cannot lose rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24657 +/- ##
==========================================
+ Coverage 81.36% 81.44% +0.07%
==========================================
Files 1117 1118 +1
Lines 397872 399611 +1739
Branches 397872 399611 +1739
==========================================
+ Hits 323725 325451 +1726
+ Misses 55229 55153 -76
- Partials 18918 19007 +89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
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.
Which issue does this PR close?
#24656
Rationale for this change
See ticket.
What changes are included in this PR?
A
DELETEor anUPDATEwhoseWHEREclause holds anINor anEXISTSsubquery changed every row of the target table, and reported the whole table as affected. The optimizer rewrites the subquery into a semi join, so the condition leaves theFilternodes thatextract_dml_filters()reads. The provider then received an empty filter list, which is the encoding for "no WHERE clause", and applied the statement to all rows.An always-false
WHEREclause reached the provider the same way. The simplifier folds the predicate into an empty relation, so again no filter survived, and aDELETE FROM t WHERE falseemptied the table.Add
classify_dml_input(), which walks the input plan of aDELETEor anUPDATEbefore the provider hook runs:The hook stays untouched in every rejected case, so a provider that writes to durable storage cannot lose rows.
Are these changes tested?
Only with the tests provided in this patch, which are based on the assumptions made in the ticket description.
Are there any user-facing changes?
?