fix(parquet): don't runtime-prune row groups while a page-index RowSelection is live (#24355) - #24359
Conversation
github-actions
Bot
added
sqllogictest
There was a problem hiding this comment.
Pull request overview
This PR prevents a parquet correctness bug where runtime dynamic row-group pruning can rebuild a push decoder while a flat, cross-row-group RowSelection is still live, causing the selection to be applied to the wrong surviving row group and yielding silently wrong results. The fix prioritizes correctness by disabling the runtime RowGroupPruner when any row_selection is present.
Changes:
- Disable runtime dynamic row-group pruning when the prepared access plan includes a
RowSelection, avoiding decoder rebuilds that can misapply selections across row groups. - Add an end-to-end sqllogictest regression reproducing #24355 and validating the correct TopK result.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| datafusion/datasource-parquet/src/opener/mod.rs | Detects presence of PreparedAccessPlan::row_selection and skips constructing the runtime RowGroupPruner to avoid selection drift on decoder rebuild. |
| datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt | Adds an SLT regression that reproduces the wrong-results scenario with page-index pruning + TopK dynamic pruning and asserts correct output. |
Suppressed comments (1)
datafusion/datasource-parquet/src/opener/mod.rs:1518
- This comment refers to a “page-index row selection”, but the guard is
has_row_selection(anyPreparedAccessPlan::row_selection). Updating the wording avoids implying only page-index-derived selections are affected.
// Also disabled when a page-index row selection is live (#24355): the
// pruner rebuilds the decoder via `with_row_groups(...)`, which drops row
// groups without slicing the carried selection to match, so pruning under
// a live selection returns wrong results. Decline to prune in that case.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…lection is live (apache#24355) The runtime row-group pruner rebuilds the decoder via into_builder().with_row_groups(new_indices), which drops row groups without slicing the carried page-index RowSelection to match. A dropped RG's selectors are then applied to the next surviving RG, silently returning wrong results. Disable the runtime pruner when a row selection is present, mirroring reorder_by_statistics which already declines to reorder in that case. The proper fix (slice the selection alongside the row groups) belongs in arrow-rs and is tracked in apache#24358. Adds an slt regression test: it fails on main (returns 50..54 instead of 0..4) and passes with this change.
f7b6cd5 to
9ba77e7
Compare
…y row_selection source The pre-existing dynamic_rg_pruning_coexists_with_page_index_row_selection test asserted the runtime pruner stays active (pruned >= 1) alongside a page-index selection. apache#24355 deliberately disables the pruner when a row selection is present, so rename it to dynamic_rg_pruning_disabled_when_page_index_row_selection_present and assert row_groups_pruned_dynamic_filter == 0 while results stay correct and page-index pruning still runs. Also reword the opener comments: a row selection comes from page-index pruning or an externally supplied ParquetRowSelection (not stats/bloom, which only skip/scan whole row groups).
9ba77e7 to
d5065af
Compare
|
Checking this one out |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @zhuqi-lucas -- this makes sense as does the proper upstream tracking fix you referenced
I think some of the comments could be cleaned up / made eaiser to understand, but that is not a blocker in my mind
| statement ok | ||
| RESET datafusion.optimizer.enable_topk_dynamic_filter_pushdown; | ||
|
|
||
| # Regression test for #24355: the runtime row-group pruner rebuilds the decoder |
There was a problem hiding this comment.
Can we please update this description to focus on what properties the test has rather than what was wrong with the current implementation (which will become immediately out of date once this PR is merged)
For example, I think the key properties of this file is that the the dynamic predicate ends up pruning a row group during the application of multiple predicates
| STORED AS PARQUET | ||
| LOCATION 'test_files/scratch/dynamic_row_group_pruning/rgsel.parquet'; | ||
|
|
||
| # The correct top-5 by `b` among rows with `a >= 50` is b = 0..4 (they live in |
There was a problem hiding this comment.
Can you also please update the test so it runs the same query without filter pushdown so it is clear the answers are the same?
| /// its `into_builder` rebuild can never drop a row group without slicing the | ||
| /// carried selection (which would silently return wrong rows). Correctness is | ||
| /// bought at the cost of the dynamic-pruning optimization for this scan; the | ||
| /// proper fix that keeps both is tracked upstream in arrow-rs #10624 / #24358. |
There was a problem hiding this comment.
can you make these actual github links (so it is clear to which repo they belong and is easier to follow the links)
| /// out those first 5 pages of RG 0 — its presence is what suppresses | ||
| /// the runtime pruner. | ||
| /// - `ORDER BY v DESC LIMIT 5` would let the tightened TopK threshold | ||
| /// (≥ 4995) prune RGs 0..3, but because a row selection is present the |
There was a problem hiding this comment.
maybe here would be a better place to add the tracking ticket for the change in behavior so it is clear what is expected to change when this feature is implemented
| // #24355: a row selection (from page-index pruning, or an externally | ||
| // supplied `ParquetRowSelection`) is carried by the decoder as one | ||
| // flat selection over the concatenation of the remaining row groups. | ||
| // The runtime pruner's `into_builder().with_row_groups(...)` rebuild |
There was a problem hiding this comment.
another good place to leave a link to the proper ticket fix
| # The correct top-5 by `b` among rows with `a >= 50` is b = 0..4 (they live in | ||
| # RG 3, all of whose rows satisfy `a >= 50`). The bug returns 50..54. | ||
| query I | ||
| SELECT b FROM rgsel WHERE a >= 50 ORDER BY b ASC LIMIT 5; |
There was a problem hiding this comment.
I reverted the code change in this PR and ran cargo test --profile=ci --test sqllogictests -- dynamic_row_group_pruning.slt
and it fails like this
. query result mismatch:
[SQL] SELECT b FROM rgsel WHERE a >= 50 ORDER BY b ASC LIMIT 5;
[Diff] (-expected|+actual)
- 0
- 1
- 2
- 3
- 4
+ 50
+ 51
+ 52
+ 53
+ 54
at /private/tmp/df-24359-ablation/datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt:252(as expected)
In case anyone else is interested in what is in the file
Details
> select * from './datafusion/sqllogictest/test_files/scratch/dynamic_row_group_pruning/rgsel.parquet';
+-----+------+
| a | b |
+-----+------+
| 100 | 1000 |
| 101 | 1001 |
| 102 | 1002 |
| 103 | 1003 |
| 104 | 1004 |
| 105 | 1005 |
| 106 | 1006 |
| 107 | 1007 |
| 108 | 1008 |
| 109 | 1009 |
| 110 | 1010 |
| 111 | 1011 |
| 112 | 1012 |
| 113 | 1013 |
| 114 | 1014 |
| 115 | 1015 |
| 116 | 1016 |
| 117 | 1017 |
| 118 | 1018 |
| 119 | 1019 |
| 120 | 1020 |
| 121 | 1021 |
| 122 | 1022 |
| 123 | 1023 |
| 124 | 1024 |
| 125 | 1025 |
| 126 | 1026 |
| 127 | 1027 |
| 128 | 1028 |
| 129 | 1029 |
| 130 | 1030 |
| 131 | 1031 |
| 132 | 1032 |
| 133 | 1033 |
| 134 | 1034 |
| 135 | 1035 |
| 136 | 1036 |
| 137 | 1037 |
| 138 | 1038 |
| 139 | 1039 |
| . |
| . |
| . |
+-----+------+And the whole results
> SELECT a, b FROM './datafusion/sqllogictest/test_files/scratch/dynamic_row_group_pruning/rgsel.parquet' WHERE a >= 50 ORDER BY b ASC LIMIT 5;
+-----+---+
| a | b |
+-----+---+
| 100 | 0 |
| 101 | 1 |
| 102 | 2 |
| 103 | 3 |
| 104 | 4 |
+-----+---+
|
Please let me know if we need to get this into the 55.0.0 release candidate since we will be making an RC3 |
i think it is needed -- I will get a clean CI run |
…trol query, use full GitHub links for tracking tickets
|
Thank you @alamb for review, addressed review comments now! |
Actually, looks like @zhuqi-lucas is on it |
|
Looks ilke a test is failing: https://github.com/apache/datafusion/actions/runs/31807076186/job/94790416372?pr=24359 |
…er still fires topk_pushdown_does_not_reread_delivered_row_group asserts the runtime pruner fires (row_groups_pruned_dynamic_filter >= 1). With page index on, the `search_phrase <> ''` filter produces an intra-RG RowSelection, and this PR disables the runtime pruner whenever a selection is present — so the test's q26 scenario no longer exercised the pruner and failed. Bug apache#24352 is row-filter-driven and does not need the page index, so disable page-index reading for this test to keep it exercising the rg_plan-sync path.
Found the root cause, we need to disable page index for previous PR sync RG PR test, addressed now, let's wait CI again. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24359 +/- ##
==========================================
- Coverage 81.19% 81.18% -0.01%
==========================================
Files 1110 1110
Lines 388616 388618 +2
Branches 388616 388618 +2
==========================================
- Hits 315529 315516 -13
- Misses 54506 54514 +8
- Partials 18581 18588 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Merged now, thanks @alamb ! |
…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
Thank you for the quick response |
Which issue does this PR close?
Rationale for this change
With
pushdown_filters = true+ dynamic filter pushdown (on by default), a querySELECT b FROM t WHERE <predicate on a> ORDER BY b LIMIT kcan silently return wrong results — rows satisfying the predicate are dropped and replaced by later ones, no error.Root cause (thanks to @adriangb's report + fixture in #24355): the push decoder carries one flat
RowSelectionover the concatenation of the remaining row groups. At a row-group boundary the runtime pruner drops row groups the dynamic predicate proves unwinnable and rebuilds the decoder:with_row_groups(new_indices)removes row groups without slicing the carriedRowSelectionto match, so the selectors intended for a dropped RG are applied to the next surviving one. In the fixture, page-index pruning leaves RG 1 withskip 50, select 50; after the TopK threshold prunes RG 1 and RG 2, the survivor RG 3 is decoded under RG 1's selection and its first 50 rows (b = 0..49, the correct answer) are wrongly skipped.This is a second, independent instance of the drift family in #24352/#24354; it is not fixed by #24354.
What changes are included in this PR?
opener/mod.rs: decline to build the runtimeRowGroupPrunerwhen a page-indexRowSelectionis present. With no pruner there is no boundary rebuild, so the carried selection is never applied to the wrong row groups. This mirrorsPreparedAccessPlan::reorder_by_statistics, which already bails when a row selection is present ("Skipping RG reorder: row_selection present") because remapping the selection is too complex.This is the minimal, DataFusion-side stop-the-bleeding fix. The proper fix is upstream in arrow-rs: apache/arrow-rs#10624 proposes letting the push decoder carry row-group-local
RowSelections (with_row_group_selections) that are preserved across rebuilds, so dropping a row group keeps every survivor's selection aligned by construction — no global-selection slicing to get wrong, and no parallelrg_planto drift (the #24352 path). DataFusion tracks that migration in #24358; this guard is removed once it lands.Are these changes tested?
dynamic_row_group_pruning.slt(the reporter's fixture viagenerate_series+COPY). It fails onmain(returns50..54instead of0..4) and passes with this change.dynamic_rg_pruning_coexists_with_page_index_row_selection,row_groups_pruned_dynamic_filter >= 1). Since this PR intentionally disables the pruner in that case, it is renamed todynamic_rg_pruning_disabled_when_page_index_row_selection_presentand now assertsrow_groups_pruned_dynamic_filter == 0while results stay correct and page-index pruning still runs. (That old test passed only because its scenario happened not to expose the bug — the misapplied selection fell outside the top-k.)Are there any user-facing changes?
Fixes silently-wrong results. Runtime row-group pruning is skipped for scans that also have a page-index row selection (correctness over a pruning optimization); this is undone once #24358 lands.
cc @alamb @adriangb @hhhizzz