adriangb commented on PR #23696:
URL: https://github.com/apache/datafusion/pull/23696#issuecomment-5346992228
@zhuqi-lucas I wanted to make sure this doesn't get lost. I think the best
way to de-risk it is to split up the PR some more to get a smaller review
surface. I worked with Claude to come up with a proposal, let me know what you
think:
### PR 1 — Strip empty row groups in `PreparedAccessPlan::prepare` (closes
#24287)
**Files:** `datafusion/datasource-parquet/src/access_plan.rs`, `src/sort.rs`
**Est.** ~110 lines changed
Land `strip_empty_row_groups` with the *generic* signature only:
```rust
fn strip_empty_row_groups(
row_group_indexes: Vec<usize>,
row_selection: Option<RowSelection>,
row_group_meta_data: &[RowGroupMetaData],
) -> (Vec<usize>, Option<RowSelection>)
```
No `fully_matched` parameter. That is exactly the decoupling #24287 asks
for, and
doing it first means it never has to be untangled later.
`sort.rs::test_prepared_access_plan_reverse_empty_selection` needs updating:
once
empty segments are stripped, an all-skipped plan strips to an *empty* plan,
so
the assertion becomes `row_group_indexes.is_empty()` and
`row_selection.is_none()`
rather than "the selection selects 0 rows".
**Why it stands alone:** it restores the 1:1 correspondence between the
prepared
plan and the readers `try_next_reader` hands back. The runtime dynamic pruner
already relies on that invariant today — this is the same family as #24352
and
#24355, both of which shipped as standalone fixes. It is a correctness fix
that
the feature happens to need, not a part of the feature.
No public API change, no new metric, no slt churn.
### PR 2 — Prebuild row-filter candidates once per file (needs a new issue)
**Files:** `src/row_filter.rs`, `src/push_decoder.rs`, `src/opener/mod.rs`
**Est.** ~230 lines changed
Add `PrebuiltRowFilterCandidate`, `prebuild_row_filter_candidates`,
`row_filter_from_prebuilt`, `RowFilterContext`,
`PrebuiltRowFilterCandidateList`;
delete `RowFilterGenerator`; have the opener prebuild once per file and
instantiate once.
**Also reimplement the existing `pub fn build_row_filter` on top of the two
new
functions.** On the current branch both paths survive — `row_filter.rs:397`
and
`row_filter.rs:502` — with duplicated conjunct splitting, ordering, and
metric
assignment. Two paths that must agree on predicate order *and* on "only the
last
predicate counts `pushdown_rows_matched`" will drift. `build_row_filter` is
public and used by the `parquet_nested_filter_pushdown` and
`parquet_struct_filter_pushdown` benches, so its signature has to stay.
Parity worth stating explicitly in the PR body: same conjunct order
(`sort_unstable_by_key(required_bytes)` when `reorder_predicates`), same
metric
wiring (every predicate shares `pushdown_rows_pruned`, only the last gets
`pushdown_rows_matched`). One genuine behavioural delta:
`reassign_expr_columns`
errors now surface once at open time instead of being swallowed per-build by
`log::debug!`.
**Why it stands alone:** today `RowFilterGenerator::build()` redoes
`split_conjunction` + `FilterCandidateBuilder::build` +
`reassign_expr_columns`
for **every row group**. Moving that to once per file is a win on any
multi-row-group scan with a pushdown predicate. It needs nothing from
`fully_matched`.
**Benchmarks:** #24328's `parquet_row_filter_skip`, plus a many-row-group
multi-conjunct case. This is the piece with a clean, attributable perf story.
### PR 3 — Extract `InitialDecoderState` (closes #24286)
**Files:** `src/opener/mod.rs`
**Est.** ~60 lines changed
Do this *before* the feature, not after. The block at `opener/mod.rs:1437`
currently returns a 3-tuple `(decoder, rg_plan, has_row_selection)`; the
feature
turns it into a 5-tuple. Landing the named struct first means the feature PR
adds
two fields to a struct instead of reshaping a tuple destructure.
Optional rider: the `prune_boundary_row_groups` extraction from
`push_decoder.rs`
is pure refactor of existing pruner code and fits the same "make the boundary
path legible" theme.
### PR 4 — The feature: skip RowFilter on fully-matched row groups (#23696)
**Est.** ~380 lines changed
What remains after the three above:
- `RgPlanEntry.fully_matched`
- `PreparedAccessPlan.fully_matched`, plus its permutation in
`reorder_by_statistics` and `reverse`
- `strip_empty_row_groups` gaining the parallel `fully_matched` vector
- `rebuild_decoder_at_boundary`, `filter_installed`, and the open-time
first-RG skip
- the `row_filter_skipped_fully_matched` metric
- `dynamic_row_group_pruning.rs` (+88) and `dynamic_row_group_pruning.slt`
(+42)
One idea, one benchmark question.
## Dependency order
```
PR1 (#24287) ───────────────────────┐
├──> PR4 (#23696, feature)
PR2 (new issue) ──> PR3 (#24286) ───┘
```
PR 1 and PR 2 are independent and can both go up immediately. PR 3 touches
the
same opener block as PR 2, so it follows PR 2. PR 4 rebases onto all three.
## Three fixes worth making while splitting
### 1. Register the metric lazily — it kills the semver flag *and* the slt
churn
`row_filter_skipped_fully_matched` is currently a `pub` field on
`ParquetFileMetrics`, created eagerly in `new()`. That is what trips
`Check semver`, and it is also why 34 unrelated EXPLAIN ANALYZE baselines
across
`push_down_filter_parquet.slt`, `explain_analyze.slt`, and
`dynamic_filter_pushdown_config.slt` had to be regenerated to carry
`row_filter_skipped_fully_matched=0`.
The sibling metric from #21637 already solves this:
`add_page_index_pages_skipped_by_fully_matched` (`metrics.rs:243`) registers
the
counter only when the value is non-zero — which is why
`page_index_pages_skipped_by_fully_matched=1` shows up in
`explain_analyze.slt`
on the one plan that actually uses it, and nowhere else.
Same pattern here: no public field, register on first suppression. The stream
needs a live handle rather than a fire-once call, so hold an `Option<Count>`
in
`PushDecoderStreamState` and build it via `MetricBuilder` the first time a
toggle
fires. Result: no public API change, no semver flag, and the slt diff
shrinks to
just the new test file.
### 2. Revert the `test_input_file_name_projection` move
In `opener/mod.rs` this test is deleted at one location and re-added ~150
lines
earlier, byte-identical. It is ~80 lines of pure diff noise that makes the
file
look far more touched than it is.
### 3. Consider one vector instead of two parallel ones
`PreparedAccessPlan::new` gets a `debug_assert_eq!(row_group_indexes.len(),
fully_matched.len())`, and `reorder_by_statistics`, `reverse`, and
`strip_empty_row_groups` each have to maintain that alignment by hand. Since
`RgPlanEntry` downstream already has exactly the `{ index, fully_matched }`
shape, it may be worth carrying one `Vec` of that struct instead — which
makes
the permutation unmisusable rather than merely asserted.
--
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]
