refactor(parquet): strip empty row groups so PreparedAccessPlan stays well-formed

[zhuqi-lucas]

Which issue does this PR close?

• Closes [Follow-up #23696] Extract generic empty-row-group stripping from strip_empty_row_groups #24287.

Rationale for this change

Split out from #23696 (per @adriangb's decomposition proposal).

This keeps PreparedAccessPlan well-formed: a row group whose RowSelection selects zero rows is dropped so with_row_groups(...) never names a row group the push decoder would immediately skip. arrow-rs advances past an all-skipped row group inside try_next_reader without handing back a reader, so keeping such a row group in the plan would leave the plan's row-group list one entry longer than the readers produced.

This is mostly hardening — the two in-tree flat-selection producers already avoid landing an empty Selection (page_filter.rs maps an all-skip selection to Skip; try_new_from_overall_row_selection normalizes via RowGroupAccessBuilder::into_access), and RowGroupPruner is gated off whenever a selection is live. There is, however, one reachable producer: ParquetAccessPlan::scan_selection intersects an existing Selection with a new one and can leave a row group selecting nothing (its rows_selected > 0 guard checks the incoming selection, not the intersection). A well-formed plan here is also a precondition for re-enabling runtime pruning under a live selection (#24358) and for #23696.

What changes are included in this PR?

• access_plan.rs: PreparedAccessPlan::prepare calls a new generic strip_empty_row_groups(row_group_indexes, row_selection, row_group_meta_data). It walks the flat RowSelection once with OverallRowSelectionCursor (the same single-pass approach try_new_from_overall_row_selection uses, rather than a per-row-group split_off) and keeps only row groups that still select a row.
• sort.rs: test_prepared_access_plan_reverse_empty_selection — an all-skipped plan now strips to an empty plan.

No fully_matched coupling, no public API change, no new metric, no slt churn.

Are these changes tested?

Yes — unit tests in access_plan.rs:

• test_strip_empty_row_groups_drops_only_empties — mixed empty/non-empty row groups.
• test_strip_empty_row_groups_non_contiguous_indexes — vec![1, 3] (RGs 0/2 skipped), pinning that row_group_meta_data is indexed absolutely.
• test_strip_empty_row_groups_none_selection_unchanged — None passes through.
• test_prepare_strips_row_group_emptied_by_intersecting_selections — the reachable scan_selection empty-intersection path, exercised through prepare().

Are there any user-facing changes?

No.

Follow-up

@adriangb also suggested a complementary one-line normalization in scan_selection itself (empty intersection → Skip), which would keep ParquetAccessPlan well-formed for its other consumers (row_group_indexes(), metrics, is_fully_matched) too. Left out here to keep this PR scoped to #24287; happy to do it as a small follow-up.

[Copilot]

Pull request overview

This PR fixes a Parquet scan bookkeeping invariant in DataFusion by proactively removing row groups whose post–page-index-pruning RowSelection selects zero rows. This prevents desynchronization between DataFusion’s prepared row-group plan / per-RG state and arrow-rs’s push decoder behavior (which can silently advance past “empty-after-pruning” row groups without returning a reader).

Changes:

• Add strip_empty_row_groups(...) in PreparedAccessPlan::prepare to split the flat RowSelection per row group and drop any row group whose segment selects zero rows.
• Add unit tests in access_plan.rs validating that only empty row groups are removed and that None selections pass through unchanged.
• Update the sort reverse test to reflect the new behavior: an all-skipped selection now strips to an empty prepared plan (row_group_indexes.is_empty() and row_selection.is_none()).

Reviewed changes

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

File	Description
datafusion/datasource-parquet/src/access_plan.rs	Introduces strip_empty_row_groups and applies it during plan preparation; adds focused unit tests.
datafusion/datasource-parquet/src/sort.rs	Updates the reverse-plan empty-selection test assertions to match the new “strip to empty plan” behavior.

---

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.82353% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.31%. Comparing base (f1f0449) to head (dd77370).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/datasource-parquet/src/access_plan.rs	98.76%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24509      +/-   ##
==========================================
+ Coverage   81.27%   81.31%   +0.03%     
==========================================
  Files        1116     1117       +1     
  Lines      395017   395987     +970     
  Branches   395017   395987     +970     
==========================================
+ Hits       321055   321987     +932     
- Misses      55166    55179      +13     
- Partials    18796    18821      +25     

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

[adriangb]

Thanks @zhuqi-lucas, this generally looks good — left a few suggestions inline.

One framing note: I think this reads better as hardening that prevents future bugs than as a live bug fix. Both in-tree producers already avoid landing an empty Selection (page_filter.rs maps an all-skip selection to RowGroupAccess::Skip, and try_new_from_overall_row_selection normalizes the same case via RowGroupAccessBuilder::into_access), and RowGroupPruner is gated off whenever a selection is live. So refactor: or minor: may be more accurate than fix: — mostly matters if anyone later considers backporting.

There is one reachable producer, though, and it's the one this PR should demonstrate — see the inline comment on the tests.

On the logic itself: reviewed at bf9336c, no blockers. It merges cleanly onto current main, and cargo fmt --check plus cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings are clean with all 33 access_plan/sort tests passing.

---

Generated by Claude Code

[adriangb]

This paragraph describes a desync that can't currently occur, which makes the function look like it's fixing a live bug rather than establishing an invariant.

Neither in-tree producer can put an empty Selection here: page_filter.rs maps an all-skip selection to access_plan.skip(row_group_index) instead of scan_selection, and try_new_from_overall_row_selection normalizes the same case through RowGroupAccessBuilder::into_access (its own test asserts RowGroupAccess::Skip for the fully-skipped group). And the "runtime dynamic-pruner" named here is switched off entirely whenever has_row_selection is true, so it never observes the misalignment.

Suggest re-scoping this to what it actually buys — the plan stays well-formed so with_row_groups(...) never names a row group the decoder will skip, which is a precondition for re-enabling pruning under a live selection (#24358) and for #23696 — rather than describing a bookkeeping failure that's already guarded.

---

Generated by Claude Code

[zhuqi-lucas]

Thanks @adriangb, good call. Reframed the doc around the well-formed-plan invariant and the one reachable producer (scan_selection intersecting to empty), and dropped the "runtime dynamic-pruner desync" description since that path is gated off. Retitled fix: → refactor: and reworded the PR body to match.

[adriangb]

Worth a note here: this indexes row_group_meta_data absolutely, while remaining only covers the row groups that are actually scanned. That pairing is correct — into_overall_row_selection emits nothing for RowGroupAccess::Skip — but it's the subtlest thing in the function, and both new tests pass vec![0, 1, 2, 3], so nothing pins it down.

A case with a non-contiguous list (e.g. vec![1, 3], RGs 0 and 2 skipped) would lock in exactly this. A one-line debug_assert! on the precondition, or a sentence saying the slice is the full file metadata, would also help — note that into_overall_row_selection uses zip in the same situation, so a short slice truncates silently there but panics here.

---

Generated by Claude Code

[zhuqi-lucas]

Thanks — added test_strip_empty_row_groups_non_contiguous_indexes (vec![1, 3], RGs 0/2 skipped) which pins the absolute indexing, and the doc now states row_group_meta_data is the full-file metadata indexed absolutely while row_selection covers only the scanned groups.

[adriangb]

This reintroduces the pattern that try_new_from_overall_row_selection was deliberately written to avoid. Its comment says it directly:

Keep this as a single pass over the selector stream rather than repeatedly calling RowSelection::split_off per row group. The split_off version is simpler, but it clones/retains substantially more selector buffer capacity for highly fragmented selections.

On a selectors-backed selection each split_off bottoms out in Vec::split_off, allocating and copying the tail, so this loop is O(row_groups × selectors). Since into_overall_row_selection always builds via collect() from a RowSelector iterator, the input is always selectors-backed and always takes that path.

OverallRowSelectionCursor is right there and would drive this in one pass — consistent with the sibling function and effectively free. This is on the per-file open path so the absolute cost is probably small, but it's the change I'd most want before merge.

---

Generated by Claude Code

[zhuqi-lucas]

Thanks, agreed — this was the one I most wanted to fix too. Rewrote it to walk the selection once with OverallRowSelectionCursor (same single-pass approach as try_new_from_overall_row_selection), so no more per-row-group split_off tail reallocation.

[adriangb]

These tests were inserted between this doc comment and the ROW_GROUP_METADATA static it describes, so /// [\RowGroupMetaData`] that returns 4 row groups with 10, 20, 30, 40 rows respectivelynow documentstest_strip_empty_row_groups_drops_only_empties`. Moving both tests below the static (line 1138) fixes it.

---

Generated by Claude Code

[zhuqi-lucas]

Thanks — moved both tests below ROW_GROUP_METADATA so its doc comment attaches to the static again.

[adriangb]

Both tests call strip_empty_row_groups directly — nothing exercises it through ParquetAccessPlan::prepare, and nothing lands in datafusion/core/tests/parquet/external_access_plan.rs.

That gap matters because there is a reachable producer of an empty Selection, and it isn't the one the doc comment names. scan_selection intersects an existing Selection with the new one and never re-normalizes an empty result back to Skip:

RowGroupAccess::Selection(existing_selection) => {
    RowGroupAccess::Selection(existing_selection.intersection(&selection))
}

So an externally supplied ParquetRowSelection plus page-index pruning whose surviving pages are disjoint from the user's selection within a row group lands an empty Selection in prepare() — the page-index path's own rows_selected > 0 guard doesn't catch it, because it checks the incoming selection, not the intersection.

That scenario is the regression test this PR wants, and it also suggests a complementary one-line fix in scan_selection itself: normalizing an empty intersection to Skip keeps ParquetAccessPlan well-formed for row_group_indexes(), the metrics, and is_fully_matched too — not just for prepare().

---

Generated by Claude Code

[zhuqi-lucas]

Great catch on the reachable producer — this is exactly the regression I was missing. Added test_prepare_strips_row_group_emptied_by_intersecting_selections, which drives the scan_selection empty-intersection case through prepare(). I left the complementary scan_selection normalization (empty intersection → Skip, which would also keep row_group_indexes() / metrics / is_fully_matched well-formed) out of this PR to keep it scoped to #24287 — happy to do it as a small follow-up.

[adriangb]

Thanks!