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:26:44 UTC
[PR] Fix/topk recursive 26642 [datafusion]
Kristofferjoha opened a new pull request, #26670:
URL: https://github.com/apache/datafusion/pull/26670
## Which issue does this PR close?
Closes #26642.
## Rationale for this change
TopK dynamic filters inside recursive CTEs stop updating the scan because `SortExec::reset_state()` creates a new filter on each iteration, while the scan still references the original one. This prevents pruning during recursive queries.
## What changes are included in this PR?
- Reset the existing shared dynamic filter instead of replacing it.
- Rebuild the TopK filter state using the existing `DynamicFilterPhysicalExpr`.
- Add a recursive TopK regression test in `cte.slt`.
Depends on #26056 for `DynamicFilterPhysicalExpr::reset()`. It hasn't been merged yet, but it has been approved, so I based this change on its implementation.
## What is the testing strategy for this PR?
The new `cte.slt` case checks that the recursive query returns 1–4 and that the Parquet scan outputs 30 rows instead of 300, pruning 27 row groups across three iterations.
## Are there any user-facing changes?
No.
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#discussion_r4370579149
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1440,7 +1440,10 @@ impl ExecutionPlan for SortExec {
.expect("rebuilt SortExec with new children")
.clone();
// Our dynamic filter and execution metrics are the state we need to reset.
- new_sort.filter = Some(new_sort.create_filter());
+ if let Some(filter) = &new_sort.filter {
+ filter.read().expr().reset()?;
Review Comment:
let me address this as well
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "Kristofferjoha (via GitHub)" <gi...@apache.org>.
Kristofferjoha commented on code in PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#discussion_r4367236581
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1440,7 +1440,12 @@ impl ExecutionPlan for SortExec {
.expect("rebuilt SortExec with new children")
.clone();
// Our dynamic filter and execution metrics are the state we need to reset.
- new_sort.filter = Some(new_sort.create_filter());
+ let expr = new_sort.filter.as_ref().map(|filter| filter.read().expr());
Review Comment:
My bad, missed the existing helper. `reset_state()` uses it now.
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#discussion_r4367055029
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1440,7 +1440,12 @@ impl ExecutionPlan for SortExec {
.expect("rebuilt SortExec with new children")
.clone();
// Our dynamic filter and execution metrics are the state we need to reset.
- new_sort.filter = Some(new_sort.create_filter());
+ let expr = new_sort.filter.as_ref().map(|filter| filter.read().expr());
Review Comment:
nit: after the reset this is the same as the existing `rebuild_filter_for_current_partitioning()` helper, so it could reuse it:
```diff
- let expr = new_sort.filter.as_ref().map(|filter| filter.read().expr());
- if let Some(expr) = expr {
- expr.reset()?;
- new_sort.filter = Some(new_sort.create_filter_with_expr(expr));
+ if let Some(filter) = &new_sort.filter {
+ filter.read().expr().reset()?;
}
-
+ new_sort.rebuild_filter_for_current_partitioning();
new_sort.metrics_set = ExecutionPlanMetricsSet::new();
```
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "Kristofferjoha (via GitHub)" <gi...@apache.org>.
Kristofferjoha commented on code in PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#discussion_r4370897880
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1440,7 +1440,10 @@ impl ExecutionPlan for SortExec {
.expect("rebuilt SortExec with new children")
.clone();
// Our dynamic filter and execution metrics are the state we need to reset.
- new_sort.filter = Some(new_sort.create_filter());
+ if let Some(filter) = &new_sort.filter {
+ filter.read().expr().reset()?;
Review Comment:
sorry didnt see it until now, Thanks
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #26670:
URL: https://github.com/apache/datafusion/pull/26670
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#discussion_r4369549545
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1440,7 +1440,10 @@ impl ExecutionPlan for SortExec {
.expect("rebuilt SortExec with new children")
.clone();
// Our dynamic filter and execution metrics are the state we need to reset.
- new_sort.filter = Some(new_sort.create_filter());
+ if let Some(filter) = &new_sort.filter {
+ filter.read().expr().reset()?;
Review Comment:
nit: #26669 makes `DynamicFilterPhysicalExpr::reset()` infallible, so this `?` won't compile once both PRs are in. The two PRs touch different files, so git won't flag a conflict. Could you drop the `?` in whichever of the two merges second (or rebase this one on #26669)?
```diff
- filter.read().expr().reset()?;
+ filter.read().expr().reset();
```
--
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 TopK dynamic filters across recursive CTE iterations [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #26670:
URL: https://github.com/apache/datafusion/pull/26670#issuecomment-6396850025
Thanks @Kristofferjoha
--
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