Skip to content

Extract RowFilterGenerator + push-decoder module from opener.rs - #6

Closed
adriangb wants to merge 3 commits into
xudong963:xudong963/prune-page-index-followupfrom
adriangb:refactor/extract-push-decoder-module
Closed

Extract RowFilterGenerator + push-decoder module from opener.rs#6
adriangb wants to merge 3 commits into
xudong963:xudong963/prune-page-index-followupfrom
adriangb:refactor/extract-push-decoder-module

Conversation

@adriangb

Copy link
Copy Markdown

Stacked on top of your apache#22191. Three small commits that move the pieces that don't depend on opener-internal types out of opener.rs, so the file gets closer to pure state-machine orchestration.

Summary

  1. Move RowFilterGenerator to row_filter.rs. Decouples it from PreparedParquetOpen — it now takes its inputs directly (predicate, schema, metadata, reorder flag, metrics), so it lives next to the build_row_filter it wraps. (I considered a literal impl From<&PreparedParquetOpen> per Adrian's suggestion, but the constructor also needs file_metadata which is a sibling of prepared rather than a field on it, so a generic new(...) was the cleanest decoupling. Happy to fold into a From<(&PreparedParquetOpen, &ParquetMetaData)> tuple impl if you'd prefer.)
  2. Extract DecoderBuilderConfig into a new push_decoder.rs module. Same struct + fields; the free build_decoder_builder becomes a .build(prepared_access_plan, metadata) method on the config, which reads more naturally at the call site.
  3. Move PushDecoderStreamState into push_decoder.rs. Co-locates the per-file stream driver with the builder configuration it consumes. Adds into_stream(self) -> BoxStream<...> so the opener doesn't need to name the unfold/fuse type — it just hands off the state and gets back a boxed stream. Drops the now-unused parquet::DecodeResult, ParquetPushDecoder, Projector, Gauge, DataFusionError, etc. imports from opener.rs.

After this, push_decoder.rs owns the full push-decoder lifecycle (builder setup + stream driving), and build_stream in opener.rs reads as orchestration: prepare access plans, build decoders, hand off to the stream, optionally wrap in EarlyStoppingStream.

The commits are stacked one-move-per-commit so each is reviewable independently.

No behavior change

Every move is pure code motion; no semantics change. The tests called out in apache#22191's body still pass:

  • cargo fmt --all --check — clean
  • cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings — clean
  • cargo test -p datafusion-datasource-parquet --lib fully_matched — 5/5 pass
  • cargo test -p datafusion-datasource-parquet --lib test_page_pruning_predicate_respects_enable_page_index — 1/1 pass

(Full --lib run has 16 unrelated failures from a missing parquet-testing submodule fixture in my worktree — same failures on the base commit.)

Are there any user-facing changes?

No. All moved items are pub(crate). The public PagePruningAccessPlanFilter::prune_plan_with_page_index API restored in apache#22191 is untouched.

adriangb added 3 commits May 14, 2026 23:41
This decouples the helper from opener.rs internals: it now takes its
inputs directly (predicate, schema, metadata, reorder flag, metrics)
rather than a `&PreparedParquetOpen`, so it lives alongside the
underlying `build_row_filter` it wraps. opener.rs constructs it from
the prepared state.

No behavior change.
Move the configuration struct and its builder construction out of
opener.rs into a new `push_decoder` module. The helper becomes a
`build(prepared_access_plan, metadata)` method on the config, which
reads more naturally at the call site.

No behavior change.
Co-locate the per-file stream driver with the builder configuration it
consumes. Add `into_stream` so the opener doesn't need to name the
unfold/fuse type; it just hands off the state and gets back a
`BoxStream`.

After this commit, push_decoder.rs owns the full push-decoder
lifecycle (builder setup + stream driving), and build_stream in
opener.rs reads as orchestration: prepare access plans, build
decoders, hand off to the stream, optionally wrap in EarlyStoppingStream.

No behavior change.
@github-actions

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale label Jul 15, 2026
@github-actions github-actions Bot closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant