michaelsembwever opened a new issue, #24998: URL: https://github.com/apache/datafusion/issues/24998
### Describe the bug `DELETE FROM t LIMIT n` deletes every row that the `WHERE` clause matches, not `n` rows. `TableProvider::delete_from(session_state, filters)` takes a filter list and nothing else, so a row count has no channel to the provider. The SQL planner does build a `Limit` node (`datafusion/sql/src/statement.rs:2288-2293`), and `extract_dml_filters()` walks past it to reach the `Filter` and `TableScan` nodes below (`datafusion/core/src/physical_planner.rs:2443`). The provider therefore sees the `WHERE` clause alone and applies it to the whole table. `UPDATE ... LIMIT` does not have the bug, because the planner rejects it: "Update-limit clause not supported" (`datafusion/sql/src/statement.rs:1168-1170`). `DELETE` accepts the clause and drops it. ### To Reproduce ```sql > create table t as values (1), (2), (3); > delete from t limit 1; +-------+ | count | +-------+ | 3 | +-------+ > select * from t; ++ ++ ``` With a `WHERE` clause the statement deletes every matching row: ```sql > create table u as values (1), (2), (3); > delete from u where column1 > 1 limit 1; +-------+ | count | +-------+ | 2 | +-------+ > select * from u; +---------+ | column1 | +---------+ | 1 | +---------+ ``` The `Limit` node is present in the plan and has no effect on the result: ``` logical_plan 01)Dml: op=[Delete] table=[t] 02)--Limit: skip=0, fetch=1 03)----Filter: t.column1 > Int64(1) 04)------TableScan: t projection=[column1] physical_plan 01)CooperativeExec 02)--DmlResultExec: rows_affected=2 ``` ### Expected behavior Either the statement deletes at most `n` rows, or DataFusion rejects it. Rejecting it is the smaller change and the consistent one. `UPDATE ... LIMIT` is already rejected, and `DELETE ... ORDER BY` is rejected too (`datafusion/sql/src/statement.rs:1207-1209`), so a `DELETE ... LIMIT n` names no row order and picks its `n` rows arbitrarily. A user who writes the clause is asking for something DataFusion cannot express. Honouring it needs a second argument on `TableProvider::delete_from`, and a decision about which rows a provider may choose when no order is given. That is a feature, and it belongs in its own issue. ### Additional context Notes for whoever takes the fix: - The check belongs next to the `UPDATE` one in `datafusion/sql/src/statement.rs`, in the `Statement::Delete` arm, so the statement fails at planning and never reaches a provider. `delete_to_plan()` then no longer needs its `limit` argument. - Pull request #24657 adds `classify_dml_input()`, which rejects a `DELETE` whose `WHERE` clause cannot reach the provider. It lets `LogicalPlan::Limit` through on purpose, with a comment pointing at this issue (`datafusion/core/src/physical_planner.rs:2333-2336`). Rejecting the clause in the SQL planner makes that arm unreachable from SQL; keep it, because a caller can still build the plan through `LogicalPlanBuilder`. - No test covers `DELETE ... LIMIT`. `dml_delete.slt` and `delete.slt` hold no case with the clause, which is why the behaviour went unnoticed. -- 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]
