Skip to content

fix: reject unsupported DELETE LIMIT - #25005

Merged
martin-g merged 9 commits into
apache:mainfrom
ryux1:fix/24998-reject-delete-limit
Oct 8, 2026
Merged

martin-g merged 9 commits into
apache:mainfrom
ryux1:fix/24998-reject-delete-limit

Conversation

@ryux1

@ryux1 ryux1 commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

DELETE with LIMIT currently appears to plan successfully, but the table provider receives only the filters and deletes every matching row. Rejecting the unsupported clause prevents a bounded delete from silently becoming an unbounded one and matches the existing UPDATE LIMIT behavior.

What changes are included in this PR?

  • reject DELETE LIMIT during SQL planning
  • remove the unreachable limit construction from delete_to_plan
  • replace the misleading EXPLAIN snapshots with error regressions, both with and without WHERE
  • directly cover both planner rejection branches in the SQL integration suite

What is the testing strategy for this PR?

  • cargo test -p datafusion-sqllogictest --test sqllogictests -- delete
  • cargo test -p datafusion-sql --lib (88 passed)
  • cargo test -p datafusion-sql --test sql_integration plan_delete_rejects_limit (2 passed)
  • cargo clippy -p datafusion-sql --all-targets -- -D warnings
  • cargo fmt --all -- --check

Are there any user-facing changes?

Yes. DELETE statements containing LIMIT now return a not-implemented planning error instead of accepting the limit and potentially deleting every matching row. There is no public Rust API change.

Implementation and validation were completed with AI coding assistance under the account owner’s direction.

@github-actions github-actions Bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Sep 7, 2026
@codecov-commenter

codecov-commenter commented Sep 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.73%. Comparing base (b7c7bc2) to head (11b72e6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25005   +/-   ##
=======================================
  Coverage   82.73%   82.73%           
=======================================
  Files        1147     1147           
  Lines      449213   449208    -5     
  Branches   449213   449208    -5     
=======================================
- Hits       371634   371632    -2     
+ Misses      54929    54925    -4     
- Partials    22650    22651    +1     

☔ 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.

@martin-g

martin-g commented Sep 7, 2026 •

Copy link
Copy Markdown
Member

Should this PR really close the issue ?
IMO the issue should stay opened until a support for limiting the number of deleted rows is implemented.
This PR is a good improvement that should be applied until the proper support is implemented.

@ryux1

ryux1 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Agreed—the issue should remain open for full bounded DELETE support. I updated the PR description so this no longer closes #24998.

@ryux1

ryux1 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

The only remaining failing check is cargo test hash collisions (amd64), which timed out at the six-hour runner limit; the rest of the matrix is green and the PR is approved. I tried to rerun the failed job, but GitHub reserves reruns here for repository admins. Could a maintainer rerun that job when convenient?

@alamb
alamb enabled auto-merge September 13, 2026 20:59
@alamb

alamb commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Thank you @ryux1 and @martin-g -- I put this in the merge queue after merging up from main

@alamb
alamb disabled auto-merge September 13, 2026 21:00

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @ryux1 and @martin-g

However, I am not sure about this one

DELETE with LIMIT currently appears to plan successfully, but the table provider receives only the filters and deletes every matching row. Rejecting the unsupported clause prevents a bounded delete from silently becoming an unbounded one and matches the existing UPDATE LIMIT behavior.

Can you please provide a reproducer that shows this bug behavior? The code in this PR appears to show a limt passed to the input.

It seems like maybe your problem is somewhere else , but just forbidding such deletes and removing the planning code doesn't see right 🤔

@ryux1

ryux1 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

I reproduced this on the exact pre-fix parent, d25ffaab4, using the CLI and an in-memory table:

create table t as values (1), (2), (3);
delete from t limit 1; -- count = 3
select * from t;       -- no rows

create table u as values (1), (2), (3);
delete from u where column1 > 1 limit 1; -- count = 2
select * from u;                         -- only 1 remains

The Limit is present in the DML input, as you noted. The loss happens later: the physical planner calls extract_dml_filters(input, table_name), which traverses through LogicalPlan::Limit but returns only Vec<Expr>. It then calls TableProvider::delete_from(session_state, filters), whose API has no limit argument. The memory provider therefore deletes every row matching those filters.

So the planner input does retain the limit, but it cannot reach the provider. The rejection in this PR prevents the observed unbounded delete until bounded-delete semantics and a provider API for them are implemented.

Comment thread datafusion/sql/src/statement.rs Outdated
Comment thread datafusion/sqllogictest/test_files/delete.slt Outdated
Comment thread datafusion/sqllogictest/test_files/delete.slt Outdated
Comment thread datafusion/sql/tests/sql_integration.rs Outdated
@martin-g

martin-g commented Oct 5, 2026

Copy link
Copy Markdown
Member

@ryux1 What do you think about my suggestions ?

Removed unsupported delete queries with limit from test file.
@martin-g

martin-g commented Oct 6, 2026

Copy link
Copy Markdown
Member

I am working on adding support for DELETE FROM tableX LIMIT .... Pull request is coming soon!

@martin-g

martin-g commented Oct 7, 2026

Copy link
Copy Markdown
Member

I am working on adding support for DELETE FROM tableX LIMIT .... Pull request is coming soon!

As explained at #24998 (comment) adding support for DELETE+LIMIT seems to be a bad idea.
At https://discord.com/channels/885562378132000778/1557010512217382962 there was a small discussion where we decided to not do it.

I am going to apply my suggestions to this PR and merge it!

martin-g and others added 4 commits October 7, 2026 13:59
This way it will be consistent with the other errors for unsupported use cases.
Also, as agreed in Discord, DELETE+LIMIT won't be supported at all in DataFusion.

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@martin-g
martin-g requested a review from alamb October 7, 2026 12:22

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ryux1 and @martin-g

@martin-g
martin-g added this pull request to the merge queue Oct 8, 2026
Merged via the queue into apache:main with commit a23b89f Oct 8, 2026
42 checks passed
Omega359 pushed a commit to Omega359/arrow-datafusion that referenced this pull request Oct 11, 2026
## Which issue does this PR close?

- Part of apache#24998; this PR adds a safe rejection until bounded DELETE is
implemented.

## Rationale for this change

DELETE with LIMIT currently appears to plan successfully, but the table
provider receives only the filters and deletes every matching row.
Rejecting the unsupported clause prevents a bounded delete from silently
becoming an unbounded one and matches the existing UPDATE LIMIT
behavior.

## What changes are included in this PR?

- reject DELETE LIMIT during SQL planning
- remove the unreachable limit construction from delete_to_plan
- replace the misleading EXPLAIN snapshots with error regressions, both
with and without WHERE
- directly cover both planner rejection branches in the SQL integration
suite

## What is the testing strategy for this PR?

- cargo test -p datafusion-sqllogictest --test sqllogictests -- delete
- cargo test -p datafusion-sql --lib (88 passed)
- cargo test -p datafusion-sql --test sql_integration
plan_delete_rejects_limit (2 passed)
- cargo clippy -p datafusion-sql --all-targets -- -D warnings
- cargo fmt --all -- --check

## Are there any user-facing changes?

Yes. DELETE statements containing LIMIT now return a not-implemented
planning error instead of accepting the limit and potentially deleting
every matching row. There is no public Rust API change.

Implementation and validation were completed with AI coding assistance
under the account owner’s direction.

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner sqllogictest SQL Logic Tests (.slt) v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants