You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Kristofferjoha (via GitHub)" <gi...@apache.org> on 2026/10/23 16:25:23 UTC
[PR] fix: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Kristofferjoha opened a new pull request, #26669:
URL: https://github.com/apache/datafusion/pull/26669
## Which issue does this PR close?
Closes #26641.
## Rationale for this change
Hash joins inside recursive CTEs read every probe-side row on each iteration because their dynamic filters stop updating the scan. This prevents pruning during recursive queries such as graph walks.
## What changes are included in this PR?
- Reset the existing shared dynamic filter instead of dropping it.
- Clear the build accumulator for each execution.
- Update prepared-build test callers to propagate reset errors.
- Add a recursive join regression test in cte.slt.
Depends on #26056 for `DynamicFilterPhysicalExpr::reset()`. It hasnt been merged yet, however it has been approved, so i tought i was safe to use its implementation.
## What is the testing strategy for this PR?
The new cte.slt case checks that the recursive query returns 1–5 and that the probe scan outputs 4 rows and prunes 396 rows across four iterations.
## Are there any user-facing changes?
The public `HashJoinExecBuilder::reset_state` method now returns `Result<Self>;` callers must handle the result.
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "Kristofferjoha (via GitHub)" <gi...@apache.org>.
Kristofferjoha commented on code in PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#discussion_r4367231235
##########
datafusion/sqllogictest/test_files/cte.slt:
##########
@@ -1727,3 +1727,107 @@ order by 1, 2;
statement ok
drop table test;
+
+# Dynamic filters produced by a recursive term must be reset between
+# iterations. Otherwise the second execution of this uncorrelated aggregate
+# reuses the first iteration's MAX bound and scans no rows.
+statement ok
+COPY (SELECT CAST(value AS DOUBLE) AS r FROM generate_series(0, 128)) TO 'test_files/scratch/cte/dynamic_filter_recursive.parquet' STORED AS PARQUET;
+
+statement ok
+CREATE EXTERNAL TABLE dynamic_filter_recursive STORED AS PARQUET LOCATION 'test_files/scratch/cte/dynamic_filter_recursive.parquet';
+
+query IR rowsort
+WITH RECURSIVE w(n, m) AS (
+ SELECT 0, CAST(NULL AS DOUBLE)
+ UNION ALL
+ SELECT w.n + 1, x.m
+ FROM w CROSS JOIN (
+ SELECT MAX(r) AS m
+ FROM dynamic_filter_recursive
+ WHERE r % 2 = 0
+ ) x
+ WHERE w.n < 3
+)
+SELECT n, m FROM w;
+----
+0 NULL
+1 128
+2 128
+3 128
+
+statement ok
+DROP TABLE dynamic_filter_recursive;
+
+# Hash join dynamic filters must remain shared with the probe scan across
+# recursive iterations (#26641). Each iteration looks up a different key, so
+# retaining a stale bound would also truncate the recursive result.
Review Comment:
Good catch, updated the comment
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#issuecomment-6395819658
I'll fix the conflicts
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#discussion_r4367010391
##########
datafusion/sqllogictest/test_files/cte.slt:
##########
@@ -1727,3 +1727,107 @@ order by 1, 2;
statement ok
drop table test;
+
+# Dynamic filters produced by a recursive term must be reset between
+# iterations. Otherwise the second execution of this uncorrelated aggregate
+# reuses the first iteration's MAX bound and scans no rows.
+statement ok
+COPY (SELECT CAST(value AS DOUBLE) AS r FROM generate_series(0, 128)) TO 'test_files/scratch/cte/dynamic_filter_recursive.parquet' STORED AS PARQUET;
+
+statement ok
+CREATE EXTERNAL TABLE dynamic_filter_recursive STORED AS PARQUET LOCATION 'test_files/scratch/cte/dynamic_filter_recursive.parquet';
+
+query IR rowsort
+WITH RECURSIVE w(n, m) AS (
+ SELECT 0, CAST(NULL AS DOUBLE)
+ UNION ALL
+ SELECT w.n + 1, x.m
+ FROM w CROSS JOIN (
+ SELECT MAX(r) AS m
+ FROM dynamic_filter_recursive
+ WHERE r % 2 = 0
+ ) x
+ WHERE w.n < 3
+)
+SELECT n, m FROM w;
+----
+0 NULL
+1 128
+2 128
+3 128
+
+statement ok
+DROP TABLE dynamic_filter_recursive;
+
+# Hash join dynamic filters must remain shared with the probe scan across
+# recursive iterations (#26641). Each iteration looks up a different key, so
+# retaining a stale bound would also truncate the recursive result.
Review Comment:
nit: I don't think the second sentence holds. When I removed `dynamic_filter.filter.reset()?` from `reset_state`, this result query still passed. Each iteration's new accumulator publishes fresh bounds before the probe scan opens, so only the `EXPLAIN ANALYZE` block below catches the missing reset. Maybe reword it so the comment says which query covers what:
```diff
# Hash join dynamic filters must remain shared with the probe scan across
-# recursive iterations (#26641). Each iteration looks up a different key, so
-# retaining a stale bound would also truncate the recursive result.
+# recursive iterations (#26641). Each iteration looks up a different key; the
+# EXPLAIN ANALYZE below checks that the filter is reset and republished, so the
+# probe scan prunes rows on every iteration.
```
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -566,11 +566,17 @@ impl HashJoinExecBuilder {
}
/// Reset task-local runtime state while retaining the immutable prepared build.
- pub fn reset_state(mut self) -> Self {
+ pub fn reset_state(mut self) -> Result<Self> {
Review Comment:
`HashJoinExecBuilder::reset_state` has been public since 53.0.0, so changing it to return `Result<Self>` is a breaking change, and it isn't declared yet (no `api change` label or upgrade-guide entry). I think we can avoid the break: the only fallible step is remapping `lit(true)` inside `DynamicFilterPhysicalExpr::reset()`, and `lit(true)` has no column references, so the remap never changes anything. Since #26056 hasn't merged yet, `reset()` could be infallible there, and then this method keeps returning `Self` (I compile-checked this and the reset tests pass):
```diff
- pub fn reset(&self) -> Result<()> {
- let new_expr = Self::remap_children(
- &self.children,
- self.remapped_children.as_ref(),
- lit(true),
- )?;
-
+ pub fn reset(&self) {
+ // `true` has no column references, so no child remapping is needed.
let mut current = self.inner.write();
let new_generation = current.generation + 1;
*current = Inner {
expression_id: current.expression_id,
generation: new_generation,
- expr: new_expr,
+ expr: lit(true),
is_complete: false,
};
drop(current);
let _ = self.state_watch.send(FilterState::InProgress {
generation: new_generation,
});
- Ok(())
}
```
```diff
- pub fn reset_state(mut self) -> Result<Self> {
+ pub fn reset_state(mut self) -> Self {
self.exec.left_fut = Default::default();
if let Some(dynamic_filter) = &mut self.exec.dynamic_filter {
- dynamic_filter.filter.reset()?;
+ dynamic_filter.filter.reset();
```
The `?` / `.unwrap()` at the other `reset()` and `reset_state()` call sites go away too. If you'd rather keep `Result`, could you add the `api change` label and a note in `docs/source/library-user-guide/upgrading/56.0.0.md`?
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#issuecomment-6395905685
<!-- 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 e3fe60c8b3326849cacd97499f2ba0971cc70d07
Building datafusion-physical-expr v55.1.0 (current)
Built [ 26.679s] (current)
Parsing datafusion-physical-expr v55.1.0 (current)
Parsed [ 0.039s] (current)
Building datafusion-physical-expr v55.1.0 (baseline)
Built [ 5.376s] (baseline)
Parsing datafusion-physical-expr v55.1.0 (baseline)
Parsed [ 0.039s] (baseline)
Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.248s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip
--- failure inherent_method_now_returns_unit: inherent method now returns unit ---
Description:
A publicly-visible method that used to return a value now returns `()`.
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/inherent_method_now_returns_unit.ron
Failed in:
datafusion_physical_expr::expressions::DynamicFilterPhysicalExpr::reset in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:349
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 33.594s] datafusion-physical-expr
Building datafusion-physical-plan v55.1.0 (current)
Built [ 19.400s] (current)
Parsing datafusion-physical-plan v55.1.0 (current)
Parsed [ 0.161s] (current)
Building datafusion-physical-plan v55.1.0 (baseline)
Built [ 8.603s] (baseline)
Parsing datafusion-physical-plan v55.1.0 (baseline)
Parsed [ 0.160s] (baseline)
Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.509s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 29.988s] datafusion-physical-plan
```
</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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "Kristofferjoha (via GitHub)" <gi...@apache.org>.
Kristofferjoha commented on code in PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#discussion_r4367230779
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -566,11 +566,17 @@ impl HashJoinExecBuilder {
}
/// Reset task-local runtime state while retaining the immutable prepared build.
- pub fn reset_state(mut self) -> Self {
+ pub fn reset_state(mut self) -> Result<Self> {
Review Comment:
Thanks, `reset()` made infallible and kept `HashJoinExecBuilder::reset_state()` returning Self. Callers updated and CTE regression and tests passes.
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "Kristofferjoha (via GitHub)" <gi...@apache.org>.
Kristofferjoha commented on PR #26669:
URL: https://github.com/apache/datafusion/pull/26669#issuecomment-6381934435
Hi @jayzhan211, I opened PRs for both #26641 and #26642. Would you be able to take a look when you have time? Both depend on #26056.
--
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: preserve hash join dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #26669:
URL: https://github.com/apache/datafusion/pull/26669
--
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