You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kakiuwang-ui (via GitHub)" <gi...@apache.org> on 2026/09/14 13:19:04 UTC
[PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
kakiuwang-ui opened a new pull request, #25294:
URL: https://github.com/apache/datafusion/pull/25294
## Which issue does this PR close?
- Closes #15046.
## Rationale for this change
An extension node can name columns whose predicates must not be pushed past it, via `UserDefinedLogicalNode::prevent_predicate_push_down_columns`. A predicate whose only dependency on those columns runs through a correlated subquery is pushed down anyway, and the resulting plan does not just perform worse — it fails to execute.
Using the `NoopPlan` node already present in the optimizer's own tests, which refuses push-down of column `c`:
```
Filter: EXISTS (SELECT sq.a FROM sq WHERE outer_ref(test.c) = sq.a)
NoopPlan
TableScan: test
```
optimizing that plan today gives
```
Optimizer rule 'push_down_filter' failed
Invalid (non-executable) plan after Optimizer rule: push_down_filter
In/Exist/SetComparison subquery can only be used in Projection, Filter, TableScan,
Window functions, Aggregate and Join plan nodes, but was used in [NoopPlan]
```
The predicate is moved below the node that asked to keep it, lands somewhere subqueries are not permitted, and the invariant check turns that into a query failure.
The cause is the one #15046 describes. The check is
```rust
!expr.column_refs().iter().any(|c| prevent_cols.contains(&c.name))
```
and `column_refs` collects only `Expr::Column`. A subquery records the outer columns it correlates on in `Subquery::outer_ref_columns`, which `Expr`'s traversal does not descend into, so `EXISTS (... WHERE outer.c = ...)` reports no column references at all and looks unconditionally safe to push.
## What changes are included in this PR?
A `references_any_column` helper in `push_down_filter.rs` that asks `column_refs` first and then, for `Expr::Exists`, `Expr::InSubquery` and `Expr::ScalarSubquery`, also looks at the `outer_ref_columns` those carry. The extension-node branch uses it instead of `column_refs` alone. Nothing else changes.
**I deliberately did not change `column_refs` itself**, which is what the issue title asks for. It has many callers whose behaviour would shift — `optimize_projections`, CSE, window and join handling all use it, and several of them want "columns produced by this plan's own schema", not outer references belonging to a parent. Widening it looks like a separate change with its own risk, so this PR fixes the consumer that has a demonstrable failure. Happy to look at the broader change instead if you would rather that were the fix.
## What is the testing strategy for this PR?
New unit test `push_down_filter::tests::user_defined_plan_outer_referenced_column`, next to the existing `user_defined_plan` test and reusing its `NoopPlan`. It asserts the `EXISTS` predicate stays above the node.
The test discriminates: with the outer-reference collection short-circuited out, it fails with the invalid-plan error quoted above; with the fix it passes. `datafusion-optimizer` (808) and `datafusion-expr` (262) pass, as does `sqllogictest`.
## Are there any user-facing changes?
No public API changes. Plans change only where an extension node declares `prevent_predicate_push_down_columns` and a predicate correlates a subquery on one of those columns — previously an invalid plan, now a correct one. No in-tree node declares those columns outside tests, so no bundled plan snapshots move.
Two adjacent things found while working on this, neither touched here:
- `find_out_reference_exprs` does not see these outer references either, for the same reason, so it cannot be used as the collection mechanism. That is the same traversal gap as #16147, which @AdamGS has taken.
- The equivalent guard for correlated subqueries in other push-down branches was not audited; this PR only covers the extension-node branch that #15046 reports.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#issuecomment-6010474739
🚀
@kakiuwang-ui
Thank you for your contribution.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #25294:
URL: https://github.com/apache/datafusion/pull/25294
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#issuecomment-5893678751
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25294?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `77.55102%` with `11 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.91%. Comparing base ([`7b00b63`](https://app.codecov.io/gh/apache/datafusion/commit/7b00b63e048b7e0de6ca42b82e0824d77862d702?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1735fc6`](https://app.codecov.io/gh/apache/datafusion/commit/1735fc6024099957456f2f54b6b43f3b04b1635c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 233 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25294?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/optimizer/src/push\_down\_filter.rs](https://app.codecov.io/gh/apache/datafusion/pull/25294?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fpush_down_filter.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3B1c2hfZG93bl9maWx0ZXIucnM=) | 77.55% | [4 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25294?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25294 +/- ##
==========================================
- Coverage 81.91% 81.91% -0.01%
==========================================
Files 1134 1134
Lines 425637 425674 +37
Branches 425637 425674 +37
==========================================
+ Hits 348654 348678 +24
- Misses 56302 56308 +6
- Partials 20681 20688 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25294?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#issuecomment-5893328187
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-optimizer v55.1.0 (current)
Built [ 34.361s] (current)
Parsing datafusion-optimizer v55.1.0 (current)
Parsed [ 0.033s] (current)
Building datafusion-optimizer v55.1.0 (baseline)
Built [ 27.913s] (baseline)
Parsing datafusion-optimizer v55.1.0 (baseline)
Parsed [ 0.034s] (baseline)
Checking datafusion-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.211s] 223 checks: 221 pass, 2 fail, 0 warn, 31 skip
--- failure module_missing: pub module removed or renamed ---
Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron
Failed in:
mod datafusion_optimizer::eliminate_aggregate_distinct, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/970b553a6e4a7de9abaae6e575098bc972621dd9/datafusion/optimizer/src/eliminate_aggregate_distinct.rs:18
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_optimizer::eliminate_aggregate_distinct::EliminateAggregateDistinct, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/970b553a6e4a7de9abaae6e575098bc972621dd9/datafusion/optimizer/src/eliminate_aggregate_distinct.rs:54
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 63.693s] datafusion-optimizer
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kakiuwang-ui (via GitHub)" <gi...@apache.org>.
kakiuwang-ui commented on PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#issuecomment-5956492798
Thanks for the ping — fixed and rebased onto main (e69e4fb).
The CI failure was a silent conflict rather than anything feature-related: `references_any_column` uses `TreeNodeRecursion`, which my branch's base still imported in `push_down_filter.rs`. On main that name was later dropped from the `datafusion_common::tree_node` import list, and since my diff never touched that line the merge took main's version — so the merged tree lost the import and failed with five `E0433: cannot find type `TreeNodeRecursion` in this scope`. That is also why the four failures were MSRV / wasm / macOS test / linux build (every job that actually compiles the crate) while fmt, semver and the doc jobs passed.
Rebasing reproduced it locally; the import is restored in the commit that introduces the function, so each commit still builds on its own.
Green locally on top of `c922f88`:
- `cargo test -p datafusion-optimizer` — 923 + 26 + 5 passed, 0 failed
- `cargo fmt -p datafusion-optimizer -- --check` — clean
- `cargo clippy -p datafusion-optimizer --all-targets -- -D warnings` — clean
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kakiuwang-ui (via GitHub)" <gi...@apache.org>.
kakiuwang-ui commented on code in PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#discussion_r4136003425
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
+ if expr.column_refs().iter().any(|c| names.contains(&c.name)) {
+ return true;
+ }
+
+ let mut found = false;
+ expr.apply(|e| {
+ let outer_refs = match e {
+ Expr::Exists(exists) => &exists.subquery.outer_ref_columns,
Review Comment:
Good catch — fixed in 479bcf5. `SetComparison` is now handled alongside the other three, and I added `user_defined_plan_outer_referenced_column_set_comparison`: `test.a > ANY (SELECT sq.a FROM sq WHERE test.c = sq.a)`, so the comparison expression names only `test.a` and the dependency on the protected `test.c` exists purely in `outer_ref_columns`.
Without the `SetComparison` arm that test fails exactly the way the original bug did:
```
Invalid (non-executable) plan after Optimizer rule: push_down_filter
In/Exist/SetComparison subquery can only be used in Projection, Filter, TableScan,
Window functions, Aggregate and Join plan nodes, but was used in [NoopPlan]
```
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
Review Comment:
Done in 479bcf5 — `Expr::Column` is now matched in the same `apply` walk, so there is no `column_refs()` `HashSet` and no second traversal. Behavior is unchanged because `add_column_refs` collects columns with that same `Expr::apply` walk; the only difference is that the column case can now stop the traversal early instead of always walking the whole expression first.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#discussion_r4134207051
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
Review Comment:
Could we check ordinary `Expr::Column` references in the existing `expr.apply` walk instead of calling `column_refs()` first? That would preserve the behavior while avoiding the temporary `HashSet` and second traversal.
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
+ if expr.column_refs().iter().any(|c| names.contains(&c.name)) {
+ return true;
+ }
+
+ let mut found = false;
+ expr.apply(|e| {
+ let outer_refs = match e {
+ Expr::Exists(exists) => &exists.subquery.outer_ref_columns,
Review Comment:
`references_any_column` handles `Exists`, `InSubquery`, and `ScalarSubquery`, but it misses `Expr::SetComparison`, which also stores correlated columns in `Subquery::outer_ref_columns`. Please include `SetComparison` here and add an `ANY` or `ALL` regression test where the comparison expression itself does not reference the protected column.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep filters with correlated subqueries above extension nodes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#issuecomment-5926947405
@kakiuwang-ui
Can you resolve the CI errors?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org