Skip to content

[branch-56] fix: give each SortExec reset_state copy its own TopK dynamic filter (backport #27142) - #27174

Merged
alamb merged 1 commit into
apache:branch-56from
raghavyadav01:backport/27142-branch-56
Nov 6, 2026
Merged

alamb merged 1 commit into
apache:branch-56from
raghavyadav01:backport/27142-branch-56

Conversation

@raghavyadav01

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

Clean cherry-pick of #27142: copies made by SortExec::reset_state() each get their own TopK dynamic filter again (reverting #26670), and the old filter is reset so a plan that is run, reset and run again doesn't keep the previous bound in the scan.

Are these changes tested?

Same tests as #27142. On branch-56: datafusion-physical-plan sort tests 164/164, sqllogictest 544/544, clippy and fmt clean.

🤖 Generated with Claude Code

…amic filter (revert apache#26670) (backport apache#27142)

## Which issue does this PR close?

- Closes apache#27130.

## Rationale for this change

As discussed in apache#27130, this reverts apache#26670 so copies made by
`SortExec::reset_state()` no longer share one TopK dynamic filter. When
a cached `LIMIT 1` plan was reset twice and both copies ran, the first
one to finish tightened the shared filter and the second returned no
rows.

A plain revert brings back a different wrong result, though: a plan that
has run once and is then reset still has its old bound in the scan's
predicate (the scan keeps the filter it got at planning time). For
`SELECT a FROM t ORDER BY a LIMIT 1` over a parquet file with pushdown
enabled, the rerun returned no rows with the plain revert. So this keeps
the in-place `reset()` from apache#26056 on the old filter and then gives the
new plan its own filter, as @Kristofferjoha suggested keeping.

| | main | plain revert | this PR |
|---|---|---|---|
| reset twice, run both copies (apache#27130) | 2nd: no rows | ok | ok |
| run, reset, run again (parquet, pushdown) | ok | 2nd: no rows | ok |

## What changes are included in this PR?

- `SortExec::reset_state` resets the old dynamic filter in place, then
creates a new one for the reset plan. The `sort_concurrency` reset from
apache#26242 is kept. (The plain revert did not apply cleanly because apache#26242
changed the same lines.)
- Removes the recursive CTE pruning test apache#26670 added to `cte.slt`,
since the scan now keeps the old filter (reset to `true`) and the
pruning no longer happens, as in 55.
- Adds two unit tests in `sorts/sort.rs`: the reproducer from apache#27130,
and a test that the filter a scan would hold is back to `true` after
`reset_state`. The second one fails with the plain revert.

## Are these changes tested?

Yes. Both new tests fail on main or on the plain revert respectively,
and pass here. All `datafusion-physical-plan` unit tests and
sqllogictests pass.

## Are there any user-facing changes?

The TopK dynamic filter inside a recursive CTE no longer prunes after
the first iteration, as in 55 (the apache#26670 test's scan reads 300 rows
instead of 30). Query results are unchanged. The proper fix (a fresh
filter per copy that the scan also picks up, which would also cover
`AggregateExec`) can follow after 56.

This reopens apache#26642.

(cherry picked from commit 25beec8)
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Nov 6, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.60656% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.26%. Comparing base (9795e8c) to head (2513184).
⚠️ Report is 3 commits behind head on branch-56.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/sorts/sort.rs 83.60% 2 Missing and 8 partials ⚠️
Flag Coverage Δ
all-tests 83.26% <83.60%> (+<0.01%) ⬆️
sqllogictests 60.73% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@             Coverage Diff             @@
##           branch-56   #27174    +/-   ##
===========================================
  Coverage      83.25%   83.26%            
===========================================
  Files           1169     1169            
  Lines         474092   474269   +177     
  Branches      474092   474269   +177     
===========================================
+ Hits          394714   394898   +184     
+ Misses         55481    55463    -18     
- Partials       23897    23908    +11     
Files with missing lines Coverage Δ
datafusion/physical-plan/src/sorts/sort.rs 88.27% <83.60%> (-0.15%) ⬇️

... and 17 files with indirect coverage changes

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

@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 @raghavyadav01

@alamb

alamb commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

I verified this is a clean cherry-pick

@alamb
alamb merged commit 6339fcb into apache:branch-56 Nov 6, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants