Fix DELETE/U{DATE filter extraction when predicates are pushed down into TableScan #19884

kosiew · 2026-01-19T09:51:45Z

Which issue does this PR close?

Closes TableProvider::delete_from problem with pushed down filters #19840.

Rationale for this change

When a TableProvider supports filter pushdown (for example TableProviderFilterPushDown::Exact), the DataFusion optimizer inlines predicates directly into the TableScan node instead of keeping them in a separate Filter node.

The existing DML planning logic only extracted predicates from explicit Filter nodes. As a result, DELETE and UPDATE operations executed against providers with filter pushdown support received no filters in delete_from / update, causing all rows to be modified instead of only the intended subset.

This change ensures that DML filter extraction correctly accounts for optimizer-pushed predicates and preserves expected semantics for DELETE and UPDATE statements.

What changes are included in this PR?

Enhanced DML filter extraction
- Updated extract_dml_filters to collect predicates from both:
  - LogicalPlan::Filter nodes
  - LogicalPlan::TableScan.filters (for pushed-down predicates)
- Split conjunctions (AND) into individual expressions consistently across both sources.
- Strip column qualifiers so expressions match the TableProvider schema.
- Deduplicate filters to avoid passing the same predicate twice when it appears in both a Filter node and a TableScan.
Improved documentation and comments
- Updated function-level documentation to explain TableScan filter handling and deduplication rationale.
- Added inline comments clarifying why deduplication is required.
Expanded test coverage
- Added test infrastructure to support configurable filter pushdown behavior in a custom TableProvider.
- Added a regression test verifying that DELETE operations correctly extract filters from TableScan when filter pushdown is enabled.
- Added a compound-filter test to ensure deduplication does not suppress distinct predicates.

Are these changes tested?

Yes. This PR includes new tests that:

Verify optimizer behavior by asserting filters are present in LogicalPlan::TableScan when pushdown is enabled.
Confirm that delete_from receives the correct number and content of filter expressions.
Validate correct handling of compound predicates (AND) without over-deduplication.

Existing DELETE and UPDATE tests continue to pass, ensuring backward compatibility when filter pushdown is not used.

Are there any user-facing changes?

Yes — this is a correctness fix.

Users implementing custom TableProviders with filter pushdown support will now receive the expected filter predicates in delete_from and update. This restores correct behavior for DELETE and UPDATE statements with WHERE clauses and prevents accidental full-table modifications.

No API changes are introduced.

LLM-generated code disclosure

This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.

Enhance DML filter extraction to incorporate table scan filters, split conjunctions, and ensure expressions are de-duplicated after stripping qualifiers. Implement exact filter pushdown handling in the delete test provider, and add a new DELETE plan test to validate that table scan filters propagate into delete_from.

mjgarton · 2026-01-19T10:43:22Z

datafusion/core/src/physical_planner.rs

+                    filters.extend(split_conjunction(filter).into_iter().cloned());
+                }
+            }
+            _ => {}


I worry slightly that as LogicalPlan and perhaps the optimiser behavior change in future, this default case may lead to similar issues. If the outcome was "nothing gets deleted" rather than "everything gets deleted" I'd worry less.

On the other hand, I can't think of how to improve it, and this change overall fixes the current issue.

Good point!

The DML input we build for DELETE/UPDATE currently only carries predicates in Filter nodes or in TableScan.filters after pushdown, so the match over those two variants is exhaustive for today's plan shapes.

We also have regression tests for pushdown paths--test_delete_filter_pushdown_extracts_table_scan_filters and `test_delete_compound_filters_with_pushdown which should catch any future optimizer change that relocates predicates away from these nodes.

If we ever add a new logical node that can hold filters, we'll extend extract_dml_filters instead of relying on the default case.

Would it make sense to handle each unhandled variant explicity instead of via a default case? This is how several of the methods in LogicalPlan work currently, presumably to deliberately cause compilation failure when new variants are added?

For example:

datafusion/datafusion/expr/src/logical_plan/plan.rs

Lines 480 to 484 in d90d074

// plans without inputs

LogicalPlan::TableScan { .. }

| LogicalPlan::EmptyRelation { .. }

| LogicalPlan::Values { .. }

| LogicalPlan::DescribeTable(_) => vec![],

Good idea - b3240bf

adriangb · 2026-01-19T17:49:05Z

Would it help to have a filters field on TableScan similar to projection?

That's something I've wanted to various reasons. My view is that if we define what a scan is as a table + a set of filter expressions + a set of projection expressions universally across the codebase a lot of use cases will be unlocked and things will be simplified. It gives us a framework under which to define cost based optimizers, etc. That becomes a digression from the issue at hand though.

ethan-tyler

Thanks for catching this @mjgarton and @kosiew for the fix! I'm the original author of this DML hooks and a little embarrassed this slipped through in my original PR. The edge case where pushdown moves predicates entirely into TableScan.filters (leaving the Filter node empty or absent) wasn't something I tested properly. Grateful for the thorough find and fix.

A few thoughts on hardening:

I would add UPDATE test coverage. extract_dml_filters handles both DELETE and UPDATE, but only DELETE is tested.
A mixed-location test would have caught my original bug. Some conjuncts in residual Filter, others in TableScan.filters.
Scope extraction to target scan only. Currently pulls from any scan in subtree and unsafe for UPDATE … FROM.
Validate before stripping qualifiers. t.id = u.id becoming id = id is dangerous. Worth a clear planning error.

Nits:

Dedup after stripping is fine for single-table DML; scoping/validation above covers multi-scan cases.
is_identity_assignment has the same qualifier-stripping hazard.

Happy to help with any of these, least I can do given I introduced the bug.

ethan-tyler · 2026-01-19T21:55:00Z

datafusion/core/src/physical_planner.rs

+                // Split AND predicates into individual expressions
+                filters.extend(split_conjunction(&filter.predicate).into_iter().cloned());
+            }
+            LogicalPlan::TableScan(TableScan {


This collects TableScan.filters from any scan in the subtree. Works for single-table DELETE/UPDATE, but unsafe for UPDATE … FROM (extra scans). Should we restrict extraction to the DML target scan (match table_name or provider identity) and fail-closed if multiple candidate scans exist?

Done - b66edf4

ethan-tyler · 2026-01-19T22:03:45Z

datafusion/core/tests/custom_sources_cases/dml_planning.rs

+async fn test_delete_filter_pushdown_extracts_table_scan_filters() -> Result<()> {
+    let provider = Arc::new(CaptureDeleteProvider::new_with_filter_pushdown(


Nice DELETE+Exact pushdown regression. I would add the same coverage for UPDATE … WHERE … + TableProviderFilterPushDown::Exact (since extract_dml_filters is used by both).

I would think about adding a "mixed location" case (some conjuncts in residual Filter, others in TableScan.filters) to lock in union+dedup behavior.

Added in 98be1b5
5fcb927

ethan-tyler · 2026-01-19T22:17:50Z

datafusion/core/src/physical_planner.rs

+    // and TableScan.filters when the optimizer pushes some predicates down.
+    // We deduplicate by (unqualified) expression to avoid passing the same filter twice.
+    let mut seen_filters = HashSet::new();
+    let deduped = filters


Dedup is done after stripping qualifiers; that can collapse distinct qualified predicates in multi-scan plans. I would either enforce single-target-scan eligibility or adjust dedup so it can't drop distinct predicates.

Addresssed via
Target Scan Scoping - b66edf4
Predicate-Level Validation - cfc945d

ethan-tyler · 2026-01-19T22:17:53Z

datafusion/core/src/physical_planner.rs

+    let mut seen_filters = HashSet::new();
+    let deduped = filters
+        .into_iter()
+        .map(strip_column_qualifiers)


Qualifier stripping is dangerous if any predicate references non-target relations (t.id = u.id → id = id). I think we should validate all Column refs belong to the DML target before stripping, and error out (fail closed) otherwise.

addressed via qualifier-validation in cfc945d

Looks good - thanks for adding target-scan scoping and the mixed residual/pushdown coverage.

One remaining concern: extract_dml_filters currently drops non-target conjuncts (predicate_is_on_target check in datafusion/core/src/physical_planner.rs:1941), and test_update_from_drops_non_target_predicates (datafusion/core/tests/custom_sources_cases/dml_planning.rs:658) codifies this as expected behavior. That can silently broaden row selection and worst case, if the join predicate is the only constraint, this becomes "update all rows".

Would it make sense to fail closed instead (planning error if any WHERE conjunct references a non-target relation), and update that test to assert the error?

ethan-tyler · 2026-01-19T22:20:39Z

datafusion/core/tests/custom_sources_cases/dml_planning.rs

        ])))))
    }
+
+    fn supports_filters_pushdown(


Right now pushdown mode is uniform for all filters. To test mixed-location behavior (Inexact vs Unsupported per conjunct), I would consider making this provider return per-filter decisions so we can force residual and pushed-down predicates in one query.

added per-filter pushdown - bc5bb4c

…ith explicit variant handling

…ider and corresponding tests

…eProvider

Update DML filter extraction to utilize TableReference matching, omit non-target predicates, and remove qualifiers post-filtering and deduplication. Pass TableReference targets into DML filter extraction for DELETE and UPDATE planning paths.

Add helper to detect table-qualified column references in captured filters for supporting UPDATE ... FROM assertions. Implement tests to verify that TableScan filters are properly extracted into DML update filters and ensure non-target predicates are dropped while retaining target filters during UPDATE ... FROM operations.

…ests

…ith single-pass `try_fold`

…tion and improved handling for UPDATE...FROM queries

…d CaptureUpdateProvider

ethan-tyler · 2026-01-20T13:00:45Z

Thanks @kosiew - I'll do another pass and look at the new TableScan.filters as well. It's coming together nicely 👌

ethan-tyler

@kosiew - Good solution for #19840, thanks for taking it on. The approach looks correct and the regression tests are solid.

Requesting changes for a couple of remaining cases that can still produce unexpected results (see inline comments).

ethan-tyler · 2026-01-21T17:18:18Z

datafusion/core/src/physical_planner.rs

+}
+
+/// Determine whether a predicate references only columns from the target table.
+fn predicate_is_on_target(expr: &Expr, target: &TableReference) -> Result<bool> {


Two concerns here:

alias-qualified columns (x.id from UPDATE t AS x) may not match target if it's the base ref -- see update.slt:91 for the plan shape

Dropping join predicates silently can broaden scope. Consider failing closed if any conjunct is rejected.

ethan-tyler · 2026-01-21T17:19:16Z

datafusion/core/tests/custom_sources_cases/dml_planning.rs

+}
+
+#[tokio::test]
+async fn test_update_from_drops_non_target_predicates() -> Result<()> {


t2 shares column names with t1, the "no t2 references" check can false-negative after qualifier stripping. I would use a t2-only column name to detect leakage reliably.

kosiew added 4 commits January 19, 2026 17:47

docs: clarify filter extraction includes TableScan and deduplication

c2fd0f8

test: add compound filter deduplication test with pushdown

7ad302d

refactor: use functional style for filter deduplication

0b71180

kosiew mentioned this pull request Jan 19, 2026

TableProvider::delete_from problem with pushed down filters #19840

Open

github-actions bot added the core Core DataFusion crate label Jan 19, 2026

mjgarton reviewed Jan 19, 2026

View reviewed changes

kosiew marked this pull request as ready for review January 19, 2026 11:28

ethan-tyler reviewed Jan 19, 2026

View reviewed changes

kosiew added 17 commits January 20, 2026 13:43

refactor: enhance filter extraction logic for various logical plans w…

b3240bf

…ith explicit variant handling

feat: add UPDATE tests , filter pushdown support to CaptureUpdateProv…

5fcb927

…ider and corresponding tests

test: add test for DELETE with mixed filter locations in CaptureDelet…

98be1b5

…eProvider

feat: enhance DELETE filter extraction to scope to target table only

b66edf4

feat: add validation and stripping of table qualifiers in DML filters

cfc945d

fix: correct string interpolation in filter validation assertion

599f0b1

Refine DML filter extraction logic

2ad307c

Update DML filter extraction to utilize TableReference matching, omit non-target predicates, and remove qualifiers post-filtering and deduplication. Pass TableReference targets into DML filter extraction for DELETE and UPDATE planning paths.

refactor: improve comments and streamline table registration in DML t…

234785e

…ests

refactor: - [ ] Replace deduplication's .collect() → .into_iter() w…

96671c7

…ith single-pass `try_fold`

refactor: enhance DML filter extraction logic with detailed documenta…

1aeb486

…tion and improved handling for UPDATE...FROM queries

Optimize predicate_is_on_target to short-circuit on first mismatch

6d7749b

Add error context when qualifier stripping fails

7aca792

clippy fix

290abd7

Add per-filter pushdown coverage

bc5bb4c

cargo fmt

84f77cf

clippy fix Refactor filter pushdown logic in CaptureDeleteProvider an…

cbd3b2e

…d CaptureUpdateProvider

kosiew mentioned this pull request Jan 20, 2026

Unified TableScan.filters #19894

Open

ethan-tyler reviewed Jan 21, 2026

View reviewed changes

	// plans without inputs
	LogicalPlan::TableScan { .. }
	\| LogicalPlan::EmptyRelation { .. }
	\| LogicalPlan::Values { .. }
	\| LogicalPlan::DescribeTable(_) => vec![],

		async fn test_delete_filter_pushdown_extracts_table_scan_filters() -> Result<()> {
		let provider = Arc::new(CaptureDeleteProvider::new_with_filter_pushdown(

Fix DELETE/U{DATE filter extraction when predicates are pushed down into TableScan #19884

Are you sure you want to change the base?

Fix DELETE/U{DATE filter extraction when predicates are pushed down into TableScan #19884

Uh oh!

Conversation

kosiew commented Jan 19, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

LLM-generated code disclosure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjgarton Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Jan 19, 2026

Uh oh!

ethan-tyler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethan-tyler commented Jan 20, 2026

Uh oh!

ethan-tyler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjgarton Jan 19, 2026 •

edited

Loading

kosiew Jan 20, 2026 •

edited

Loading

kosiew Jan 20, 2026 •

edited

Loading