zhuqi-lucas opened a new pull request, #24359: URL: https://github.com/apache/datafusion/pull/24359
## Which issue does this PR close? - Closes #24355. ## Rationale for this change With `pushdown_filters = true` + dynamic filter pushdown (on by default), a query `SELECT b FROM t WHERE <predicate on a> ORDER BY b LIMIT k` can 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 `RowSelection` over 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: ```rust decoder.into_builder()?.with_row_groups(new_indices).build() ``` `with_row_groups(new_indices)` removes row groups **without slicing the carried `RowSelection` to 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 with `skip 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 runtime `RowGroupPruner` when a page-index `RowSelection` is present.** With no pruner there is no boundary rebuild, so the carried selection is never applied to the wrong row groups. This mirrors `PreparedAccessPlan::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 — slicing the `RowSelection` alongside the row-group set on rebuild (e.g. an arrow-rs `retain_row_groups`) — is tracked as a structural follow-up in #24358, which would also remove the parallel `rg_plan` state behind #24352. ## Are these changes tested? - Adds an slt regression test in `dynamic_row_group_pruning.slt` (the reporter's fixture via `generate_series` + `COPY`). It **fails on `main`** (returns `50..54` instead of `0..4`) and passes with this change. The existing dynamic-prune tests still pass — they have no row selection, so the pruner is created as before. ## 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
