Skip to content

fix(catalog): apply a MemTable DELETE or UPDATE when the plan runs - #24655

Open
michaelsembwever wants to merge 1 commit into
apache:mainfrom
thelastpickle:fix/memtable-dml-execute-time
Open

fix(catalog): apply a MemTable DELETE or UPDATE when the plan runs#24655
michaelsembwever wants to merge 1 commit into
apache:mainfrom
thelastpickle:fix/memtable-dml-execute-time

Conversation

@michaelsembwever

Copy link
Copy Markdown
Member

Which issue does this PR close?

#24654

Rationale for this change

What changes are included in this PR?

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.

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?

`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>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.44%. Comparing base (a6e2d3f) to head (8e695f6).
⚠️ Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/catalog/src/memory/table.rs 85.89% 10 Missing and 12 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

catalog Related to the catalog crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants