[branch-55] fix wrong TopK results from re-reading already-delivered row groups (#24352) by zhuqi-lucas · Pull Request #24368 · apache/datafusion

zhuqi-lucas · 2026-08-14T13:26:03Z

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

@hhhizzz

…ts from re-reading already-delivered row groups (apache#24352) (apache#24354) ## Which issue does this PR close? - Closes apache#24352. ## Rationale for this change With `datafusion.execution.parquet.pushdown_filters = true` and TopK dynamic filter pushdown (both on by default), a query of the shape `SELECT b FROM t WHERE <predicate on a> ORDER BY b LIMIT k` can silently return **wrong results** — one source row emitted several times and the true tail of the top-k missing — with no error or warning. Root cause (thanks to @hhhizzz's very detailed report + fixture in apache#24352): a row group whose post-predicate selection is empty is silently finished by arrow-rs **without handing back a reader**. `PushDecoderStreamState` pops its `rg_plan` **only** when a reader is returned, so after a silently-finished RG the plan trails the decoder by one. When the runtime row-group pruner then rebuilds the decoder (`into_builder().with_row_groups(...)`) from the stale `rg_plan`, it re-includes an already-delivered row group, whose rows are emitted a second time and displace the genuine top-k in the heap. ## What changes are included in this PR? - `push_decoder.rs`: before each boundary prune/rebuild, `rg_plan` is synced to the row group the decoder will actually emit next via `peek_next_row_group()` (`sync_rg_plan_to_decoder_frontier` / `advance_rg_plan_to`), dropping entries for silently-finished row groups so a rebuild can never re-include a delivered group. A rebuild frontier naming an RG not in the plan is now an internal error instead of a silent plan drain. ## Are these changes tested? - Adds @hhhizzz's fixture as an slt regression test in `dynamic_row_group_pruning.slt` (filter column `search_phrase` differs from the sort column `event_time`, one row group has an empty post-predicate selection invisible to statistics). It now returns the correct `p0 p4096 p4097 … p4104` (was the buggy `p0 p4096 p4096 …`). - clippy clean; `datasource-parquet` unit tests and the sqllogictest suite pass locally. ## Are there any user-facing changes? Fixes silently-wrong query results; no API change. ## Note This is the standalone bug fix extracted from apache#23696 (per review discussion in apache#24352): the same `rg_plan` ↔ decoder-frontier sync, on its own so it merges fast and is easy to backport. apache#23696 will rebase on top so it carries only the fully-matched `RowFilter` skip performance optimization. cc @alamb @adriangb @hhhizzz (cherry picked from commit 574fe67)

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 · 2026-08-14T13:44:14Z

Let's wait for CI to finish before merging

alamb · 2026-08-14T13:52:28Z

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

codecov-commenter · 2026-08-14T13:54:08Z

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 · 2026-08-14T13:58:23Z

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

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

zhuqi-lucas · 2026-08-14T13:59:03Z

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

@timsaucer

…ection is live (#24355) (#24374) Backport of #24359 to `branch-55` for the 55.0.0 release, per @timsaucer's request in #22393. Stacks cleanly on the already-merged #24368 (#24354 backport). ## Which issue does this PR close? - Backports the fix for #24355 — a second, independent silent wrong-results bug in the same parquet dynamic row-group pruning path as #24352. ## Rationale With `pushdown_filters=true` + a TopK dynamic filter, the runtime row-group pruner rebuilds the push decoder via `into_builder().with_row_groups(...)`, which drops row groups **without slicing** the carried flat page-index `RowSelection` to match — a dropped RG's selectors are then applied to the next surviving RG, silently returning wrong rows (no error). The fix declines to build the runtime `RowGroupPruner` when a row selection is present (correctness over the pruning optimization); the proper fix that keeps both is tracked upstream in apache/arrow-rs#10624 / #24358. ## Notes - Clean cherry-pick of #24359 onto `branch-55` (which now has #24354 via #24368). No conflicts. - #24359 is **approved** on `main` and pending merge; opening this now so it can ride RC3. - Verified locally on this branch: the full `dynamic_row_group_pruning` rust module (9/9) and `dynamic_row_group_pruning.slt` pass; clippy clean. cc @timsaucer @alamb @adriangb

Copilot AI lite review requested due to automatic review settings August 14, 2026 13:26

github-actions Bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) datasource Changes to the datasource crate labels Aug 14, 2026

zhuqi-lucas requested review from adriangb, alamb and timsaucer August 14, 2026 13:26

Copilot started reviewing on behalf of zhuqi-lucas August 14, 2026 13:27 View session

zhuqi-lucas mentioned this pull request Aug 14, 2026

Release DataFusion 55.0.0 (Jul / Aug 2026) #22393

Open

50 tasks

Copilot AI reviewed Aug 14, 2026

View reviewed changes

timsaucer approved these changes Aug 14, 2026

View reviewed changes

timsaucer changed the title ~~Backport #24354 to branch-55: fix wrong TopK results from re-reading already-delivered row groups (#24352)~~ [branch-55] fix wrong TopK results from re-reading already-delivered row groups (#24352) Aug 14, 2026

zhuqi-lucas merged commit 26c0204 into apache:branch-55 Aug 14, 2026
35 checks passed

zhuqi-lucas mentioned this pull request Aug 14, 2026

[branch-55]: don't runtime-prune row groups while a page-index RowSelection is live (#24355) #24374

Merged

Navigation Menu

Uh oh!

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

[branch-55] fix wrong TopK results from re-reading already-delivered row groups (#24352)#24368
zhuqi-lucas merged 1 commit into
apache:branch-55from
zhuqi-lucas:backport-24354-branch-55

zhuqi-lucas commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

timsaucer commented Aug 14, 2026

Uh oh!

alamb commented Aug 14, 2026

Uh oh!

codecov-commenter commented Aug 14, 2026

Uh oh!

zhuqi-lucas commented Aug 14, 2026

Uh oh!

zhuqi-lucas commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

zhuqi-lucas commented Aug 14, 2026

Which issue does this PR close?

Rationale

What changes are included?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

timsaucer commented Aug 14, 2026

Uh oh!

alamb commented Aug 14, 2026

Uh oh!

codecov-commenter commented Aug 14, 2026

Codecov Report

Uh oh!

zhuqi-lucas commented Aug 14, 2026

Uh oh!

zhuqi-lucas commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants