michaelsembwever opened a new issue, #24656: URL: https://github.com/apache/datafusion/issues/24656
### Describe the bug `EXPLAIN DELETE` and `EXPLAIN UPDATE` change the rows of an in-memory table. The plan is printed, and the statement has also run. Two causes combine. 1. `handle_explain()` builds the physical plan of the statement in order to render the `physical_plan` section (`datafusion/core/src/physical_planner.rs:2729-2733` for the indent format, `:2678-2686` for the tree format). Only the `PostgresJSON` and `Graphviz` formats stop at the logical plan. 2. The physical planner calls the provider hook while it builds the plan. The `WriteOp::Delete` arm awaits `TableProvider::delete_from()` (`:791-812`), and the `WriteOp::Update` arm awaits `TableProvider::update()` (`:813-838`). `MemTable` then does the whole row change inside the hook. `delete_from_inner()` takes a write lock on each partition and overwrites it with the surviving batches, then returns `DmlResultExec::new(total_deleted)` (`datafusion/catalog/src/memory/table.rs:367-428`). `update_inner()` does the same (`:440-572`). `DmlResultExec` is a constant node: its `execute()` only emits the count that the hook already computed (`:702-720`). The hook also clears the declared sort order of the table (`:376`, `:483`), which is a second observable effect of the `EXPLAIN`. `delete.slt` carries the evidence. Every `explain delete` case prints `02)--DmlResultExec: rows_affected=0`, and the count is baked into the plan text because the delete already ran against the empty table `t1`. ### To Reproduce ```sql > create table t as values (1), (2), (3); > explain delete from t where column1 > 1; +---------------+-------------------------------------+ | plan_type | plan | +---------------+-------------------------------------+ | logical_plan | Dml: op=[Delete] table=[t] | | | Filter: t.column1 > Int64(1) | | | TableScan: t | | physical_plan | CooperativeExec | | | DmlResultExec: rows_affected=2 | +---------------+-------------------------------------+ > select * from t; +---------+ | column1 | +---------+ | 1 | +---------+ ``` The rows are gone, and the count appears in the plan text rather than in a result. ### Expected behavior `EXPLAIN DELETE` and `EXPLAIN UPDATE` print the plan and change nothing. The three rows survive. `EXPLAIN ANALYZE DELETE` is expected to change the rows: it runs the plan by design, and it must run the statement exactly once. The provider documentation states the rule that the fix should follow: do the row change in the `execute()` method of the plan that the hook returns, so the hook stays lightweight. `MemTable` should follow its own guidance, since the documentation names it as the reference implementation. ### Additional context The documentation added by pull request #24567 records the behaviour as a warning in two places, and neither cites a tracker issue: - `docs/source/user-guide/sql/dml.md`: "`EXPLAIN` executes a `DELETE` or an `UPDATE` on an in-memory table ... Use a copy of the table if you want to read the plan only." - `docs/source/library-user-guide/custom-table-providers.md`: "The hooks run during physical planning, like `scan()`. A hook that changes rows before it returns its plan therefore changes them during planning, and `EXPLAIN DELETE` or `EXPLAIN UPDATE` also changes them. MemTable works this way." Notes for whoever takes the fix: - No test asserts that `EXPLAIN` leaves the data alone. `delete.slt` and `update.slt` hold the `EXPLAIN` cases and pass only because their tables are empty. `dml_delete.slt` and `dml_update.slt` hold the behavioural cases and use no `EXPLAIN`. - The mutation moves out of the hook, so the row count is no longer known while the plan is built. The display text of the new plan node cannot carry `rows_affected`, and the eight expectations in `delete.slt` and `update.slt` need regenerating. - The two fields of `MemTable` that the operation needs, `batches` and `sort_order`, already sit behind an `Arc`, so the plan node can hold clones and apply the change in `execute()`. - A plan that changes rows in `execute()` changes them once per run, so a caller that runs the plan twice applies the statement twice. `DataSinkExec` behaves the same way for an `INSERT`. - Keep every planning error in the hook. The `UPDATE` check for an unknown column raises a plan error today (`datafusion/catalog/src/memory/table.rs:460-466`), and `EXPLAIN` should still report it. - `insert_into()` clears the sort order inside the hook as well (`:344`), so `EXPLAIN INSERT` keeps that smaller effect. Worth a follow-up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
