fix(catalog): apply a MemTable DELETE or UPDATE when the plan runs - #24655
Open
michaelsembwever wants to merge 1 commit into
Open
fix(catalog): apply a MemTable DELETE or UPDATE when the plan runs#24655michaelsembwever wants to merge 1 commit into
michaelsembwever wants to merge 1 commit into
Conversation
`EXPLAIN DELETE` and `EXPLAIN UPDATE` changed the rows of a `MemTable`. `handle_explain()` builds the physical plan in order to print it, the physical planner calls the provider hook while it builds the plan, and `MemTable` did the whole row change inside the hook. The returned `DmlResultExec` was a constant node that only reported the count the hook had computed, so the plan text also carried the count. Replace `DmlResultExec` with `MemDmlExec`. The hook now compiles the `WHERE` clause and the assignments, then returns a plan that holds the partitions and the declared sort order of the table. `execute()` applies the operation, clears the sort order, and emits the count. This is the pattern that the provider guide already recommends, and `MemTable` is the reference implementation. Every check of the statement stays in the hook, so an `EXPLAIN` still reports an invalid statement. A plan that runs twice applies the operation twice, as `DataSinkExec` does for an INSERT. The `DmlResultExec: rows_affected=0` lines of `delete.slt` and `update.slt` become `MemDmlExec: op=Delete` and `MemDmlExec: op=Update`. The count is unknown while the plan is built, so it no longer appears in the plan text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24655 +/- ##
==========================================
+ Coverage 81.36% 81.44% +0.08%
==========================================
Files 1117 1118 +1
Lines 397872 399600 +1728
Branches 397872 399600 +1728
==========================================
+ Hits 323725 325458 +1733
+ Misses 55229 55137 -92
- Partials 18918 19005 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
martin-g
approved these changes
Aug 27, 2026
alamb
approved these changes
Aug 27, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Makes sense to me -- thank you @michaelsembwever and @martin-g
| )?; | ||
| /// Delete the rows of `state` that its filters match, and return the number of | ||
| /// rows deleted. | ||
| async fn apply_delete(state: &MemDmlState) -> Result<u64> { |
Contributor
There was a problem hiding this comment.
minor: would this be more natural as a method of MemDmlState rather than taking it as an argument ?
impl MemDmlState {
fn apply_delete(&self) -> Result<u64> {
...
}
}| /// Assign a new value to each row of `state` that its filters match, and return | ||
| /// the number of rows updated. | ||
| async fn apply_update( | ||
| state: &MemDmlState, |
Contributor
There was a problem hiding this comment.
similar question above -- maybe this would be nicer as a MemDmlState::apply_update method
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
4 participants
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?
Rationale for this change
What changes are included in this PR?
EXPLAIN DELETEandEXPLAIN UPDATEchanged the rows of aMemTable.handle_explain()builds the physical plan in order to print it, the physical planner calls the provider hook while it builds the plan, andMemTabledid the whole row change inside the hook. The returnedDmlResultExecwas a constant node that only reported the count the hook had computed, so the plan text also carried the count.Replace
DmlResultExecwithMemDmlExec. The hook now compiles theWHEREclause and the assignments, then returns a plan that holds the partitions and the declared sort order of the table.execute()applies the operation, clears the sort order, and emits the count. This is the pattern that the provider guide already recommends, andMemTableis the reference implementation.Every check of the statement stays in the hook, so an
EXPLAINstill reports an invalid statement. A plan that runs twice applies the operation twice, asDataSinkExecdoes for an INSERT.The
DmlResultExec: rows_affected=0lines ofdelete.sltandupdate.sltbecomeMemDmlExec: op=DeleteandMemDmlExec: op=Update. The count is unknown while the plan is built, so it no longer appears in the plan text.Are these changes tested?
Only with the tests here, which are based on the assumptions made in the issue.
Are there any user-facing changes?