[branch-55]: don't runtime-prune row groups while a page-index RowSelection is live (#24355) - #24374
Conversation
…lection is live (apache#24355) Backport of apache#24359 to branch-55. 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 results. Decline to build the runtime RowGroupPruner when a row selection is present (mirroring reorder_by_statistics). Proper fix tracked in arrow-rs apache#10624 / apache#24358.
There was a problem hiding this comment.
Pull request overview
Backports the fix for DataFusion issue #24355 to branch-55 by preventing parquet runtime row-group pruning when a page-index (or externally supplied) RowSelection is present, avoiding silent wrong-results caused by misaligned selections after decoder rebuilds.
Changes:
- Disable construction of the runtime
RowGroupPrunerwhenPreparedAccessPlan.row_selection.is_some()to preventinto_builder().with_row_groups(...)from dropping row groups without remapping the carried selection. - Add an SLT regression test reproducing the wrong-results scenario (
a >= 50with TopK + page-index pruning) and asserting correct output. - Update/rename the Rust integration test to assert the new intended behavior (page-index pruning still runs, runtime dynamic RG pruning is skipped), and adjust the #24352 regression test to disable page-index reading so it still exercises runtime pruning.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| datafusion/datasource-parquet/src/opener/mod.rs | Adds a guard to skip runtime dynamic row-group pruning while a RowSelection is live, preventing silent misalignment bugs. |
| datafusion/core/tests/parquet/dynamic_row_group_pruning.rs | Updates regression coverage to reflect the new behavior (pruner disabled under row selection) and keeps #24352 test exercising runtime pruning. |
| datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt | Adds an end-to-end SLT regression reproducer for the page-index RowSelection + TopK dynamic pruning wrong-results case. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks @alamb for review, merged now. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## branch-55 #24374 +/- ##
=============================================
- Coverage 81.14% 81.14% -0.01%
=============================================
Files 1110 1110
Lines 386182 386184 +2
Branches 386182 386184 +2
=============================================
- Hits 313370 313363 -7
- Misses 54344 54352 +8
- Partials 18468 18469 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Backport of #24359 to
branch-55for 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?
Rationale
With
pushdown_filters=true+ a TopK dynamic filter, the runtime row-group pruner rebuilds the push decoder viainto_builder().with_row_groups(...), which drops row groups without slicing the carried flat page-indexRowSelectionto 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 runtimeRowGroupPrunerwhen 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
branch-55(which now has fix(parquet): sync rg_plan to decoder frontier — fix wrong TopK results from re-reading already-delivered row groups (#24352) #24354 via [branch-55] fix wrong TopK results from re-reading already-delivered row groups (#24352) #24368). No conflicts.mainand pending merge; opening this now so it can ride RC3.dynamic_row_group_pruningrust module (9/9) anddynamic_row_group_pruning.sltpass; clippy clean.cc @timsaucer @alamb @adriangb