michaelsembwever opened a new pull request, #25040: URL: https://github.com/apache/datafusion/pull/25040
## Which issue does this PR close? - Closes #24656. ## Rationale for this change `EXPLAIN DELETE` and `EXPLAIN UPDATE` changed the rows of an in-memory table. The plan was printed, and the statement had also run: ```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 | +---------+ ``` Two causes combined. `handle_explain()` builds the physical plan in order to print the `physical_plan` section, and the planner awaits `TableProvider::delete_from()` and `TableProvider::update()` while it builds. `MemTable` did the whole row change inside those hooks: it took a write lock on each partition, overwrote it, cleared the declared sort order, and returned a constant `DmlResultExec` carrying a count it had already computed. The row count baked into the plan text was the tell. `EXPLAIN ANALYZE DELETE` is expected to change the rows, because it runs the plan by design. It must apply the statement exactly once. ## What changes are included in this PR? Each hook is split into a planning half and an execution half. | Stays in the hook (planning) | Moves to `execute()` | |---|---| | Clone the `batches` and `sort_order` handles | Take the write lock on each partition | | Build the physical predicates of the `WHERE` clause | Evaluate the mask per batch | | Validate the `SET` column names, build the physical assignments | Rewrite or filter the batches | | | Clear the declared sort order | | | Count the affected rows and emit the `count` batch | `delete_from()` now returns `MemDeleteExec` and `update()` returns `MemUpdateExec`. Each node holds the shared partition handles plus the expressions the hook built, and applies the change on the first poll of the stream that `execute()` returns. `DmlResultExec` is removed, since the count is not known while the plan is built. Each run of the plan applies the statement once, as `DataSinkExec` does for an `INSERT`. The row logic itself moved unchanged into `delete_rows()` and `update_rows()`, including the SQL three-valued logic for a NULL predicate and the `evaluate_selection` call that keeps an error such as a divide by zero away from the rows the statement does not touch. Planning errors stay in the hooks, so `EXPLAIN` still reports an unknown `SET` column and a predicate that cannot be planned. `apply_expressions()` now visits the expressions the nodes hold; the constant node it replaces had none. Both nodes are private to `datafusion-catalog`, so there is no public API change. ## What is the testing strategy for this PR? New sqllogictest cases in `dml_delete.slt` and `dml_update.slt` assert that `EXPLAIN` leaves the three rows alone and that `EXPLAIN ANALYZE` changes them. Against the unfixed provider both fail. The update case adds ten to each matching value rather than deleting, so a plan that ran twice would print `22` and `23` instead of `12` and `13`; that is what pins the once-only guarantee, and it is what the old code produced. The eight physical plan expectations in `delete.slt` and `update.slt` are regenerated. They can no longer carry `rows_affected`, and the new text names the predicates and the assignments instead: ``` 02)--MemDeleteExec: predicate=[CAST(a@0 AS Int64) = 1, c@2 > CAST(3 AS Float64)] 02)--MemUpdateExec: set=[a=CAST(c@2 + CAST(1 AS Float64) AS Int32), b=CAST(a@0 AS Utf8View)] ``` `cargo test --test sqllogictests`, `cargo test -p datafusion --lib`, and `cargo test -p datafusion --test core_integration` show identical results before and after the change. Clippy passes with `-D warnings` on `datafusion-catalog`, `datafusion`, and `datafusion-sqllogictest`. ## Are there any user-facing changes? Yes, and all three are the point of the fix or follow from it. `EXPLAIN DELETE` and `EXPLAIN UPDATE` on a `MemTable` no longer change data, and they no longer clear the table's declared sort order. The physical plan text of a `DELETE` or an `UPDATE` on a `MemTable` changed. `DmlResultExec: rows_affected=N` becomes `MemDeleteExec` or `MemUpdateExec`, which name the predicates and the assignments in place of the count. One behaviour change beyond the fix, noted for completeness: the early return for a `MemTable` with no partitions is gone, so an `UPDATE` naming an unknown column now raises its plan error rather than reporting zero rows. `MemTable::try_new` rejects an empty partition list, so this is unreachable through normal construction. `EXPLAIN INSERT` still clears the declared sort order, because `insert_into()` does that inside the hook. That is a smaller instance of the same shape and is left for 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]
