[branch-55] fix wrong TopK results from re-reading already-delivered row groups (#24352)

[zhuqi-lucas]

Backport of #24354 to branch-55 for the 55.0.0 release, per @timsaucer's request in #22393.

Which issue does this PR close?

• Backports the fix for Wrong results: TopK dynamic filter + pushdown_filters re-reads already-delivered row groups (duplicate rows displace the true top-k) #24352 (wrong TopK results from re-reading already-delivered row groups).

Rationale

#24352 is a silent wrong-results bug: with pushdown_filters=true + TopK dynamic filter pushdown (both on by default), a row group whose post-predicate selection is empty is finished by arrow-rs without handing back a reader, so DataFusion's rg_plan trails the decoder frontier by one and a later runtime prune rebuilds the decoder from a stale plan — re-reading an already-delivered row group, duplicating rows and dropping the true top-k tail. No error is raised.

This is a clean cherry-pick of the squashed #24354 commit (574fe67); it applies to branch-55 without conflicts.

What changes are included?

push_decoder.rs: sync rg_plan to the decoder frontier via peek_next_row_group() before each runtime prune/rebuild (gated on row_group_pruner.is_some() so ordinary scans pay nothing), with a defensive internal_err! if the frontier diverges from the plan. Plus the slt + rust regression tests from #24354.

cc @timsaucer@alamb@adriangb

[Copilot]

Pull request overview

Backports the fix for a silent wrong-results bug in the Parquet push-decoder path on branch-55, ensuring runtime row-group prune/rebuild cannot re-read already-delivered row groups when pushdown_filters and TopK dynamic filter pushdown are enabled.

Changes:

• Synchronize rg_plan with the decoder’s actual frontier via peek_next_row_group() before any runtime prune/rebuild, and surface divergence as an internal error.
• Add an end-to-end sqllogictest regression reproducer for #24352.
• Add a Rust end-to-end regression test that asserts both correct TopK output and that dynamic RG pruning actually occurred.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
datafusion/datasource-parquet/src/push_decoder.rs	Sync rg_plan to decoder frontier prior to runtime prune/rebuild to prevent re-reading previously delivered row groups; add unit tests for plan advancement behavior.
datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt	Adds SLT regression that writes a crafted Parquet fixture and verifies correct TopK output under pushdown_filters + dynamic filter pushdown.
datafusion/core/tests/parquet/dynamic_row_group_pruning.rs	Adds Rust regression test with the same fixture characteristics, asserting correct output and that runtime dynamic pruning was exercised.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[timsaucer]

Let's wait for CI to finish before merging

[alamb]

Should we also backport the fix for this one: #24359 ?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.14%. Comparing base (83d3489) to head (328b92a).
⚠️ Report is 2 commits behind head on branch-55.

Files with missing lines	Patch %	Lines
datafusion/datasource-parquet/src/push_decoder.rs	91.48%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## branch-55 #24368 +/- ##
=============================================
- Coverage 81.14% 81.14% -0.01% 
=============================================
Files 1110 1110 Lines 386132 386179 +47 Branches 386132 386179 +47 =============================================
+ Hits 313325 313362 +37 - Misses 54340 54344 +4 - Partials 18467 18473 +6 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

[zhuqi-lucas]

Should we also backport the fix for this one: #24359 ?

Yes @alamb , i will do this after it merged!

[zhuqi-lucas]

Merging this PR now @timsaucer@alamb , CI has passed.