BUG: DELETE with a LIMIT clause ignores the LIMIT and deletes every matching row
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### 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.
Contributor guide
Research direction
Start in datafusion/sql/src/statement.rs near the existing UPDATE and DELETE clause checks, then inspect delete_to_plan(). Add regression coverage in dml_delete.slt or delete.slt for DELETE ... LIMIT. Done means the statement is rejected during planning rather than reaching the provider, while the LogicalPlanBuilder path remains supported.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 86/100