Uh oh!
There was an error while loading. Please reload this page.
refactor(parquet-datasource): mechanical cleanup of opener / file_format / row_group_filter - #22156
Conversation
alamb
commented
May 13, 2026
We could potentially split this into module moves too |
b044e05 to
3efc367Compare3efc367 to
17679e3Compare17679e3 to
2706027Compare…ile_format.rs, row_group_filter.rs A series of pure code-motion splits to make the parquet datasource crate easier to navigate and modify. No public API changes (existing import paths preserved via re-exports), no behavior changes. Five separate moves: 1. `opener.rs` (~2,700 LOC) → `opener/` module: - `opener/early_stop.rs` — `EarlyStoppingStream`, the dynamic-filter early-termination wrapper used at the end of `build_stream`. - `opener/encryption.rs` — `EncryptionContext` + the `ParquetMorselizer::get_encryption_context` helpers. Isolates the `#[cfg(feature = "parquet_encryption")]` gating. - `opener/push_decoder_stream.rs` — `PushDecoderStreamState`, the inner stream state machine that drives a `ParquetPushDecoder`. 2. `file_format.rs` (~2,000 LOC) split: - `sink.rs` — `ParquetSink` + the parallel-write machinery (`column_serializer_task`, `spawn_column_parallel_row_group_writer`, `output_single_parquet_file_parallelized`, etc.). The historical `file_format::ParquetSink` path is preserved via `pub use`. - `schema_coercion.rs` — Arrow-schema coercion utilities (`apply_file_schema_type_coercions`, `coerce_int96_to_resolution`, `coerce_file_schema_to_view_type`, `coerce_file_schema_to_string_type`, `transform_schema_to_view`, `transform_binary_to_string`, `field_with_new_type`) and their tests. Re-exported at the crate root for backward compat. 3. `row_group_filter.rs` (~1,900 LOC): - `bloom_filter.rs` — `BloomFilterStatistics` (the loaded SBBF data + its `PruningStatistics` adapter). Separates "data we loaded from the file" from "the access-plan filter that consumes it". After this PR: - `opener.rs`: 2,717 → 2,433 LOC (-10%; further drop possible by extracting the test module, but tests stay near the code under test for now). - `file_format.rs`: 2,038 → 633 LOC (-69%). - `row_group_filter.rs`: 1,929 → 1,756 LOC (-9%; bloom code moved out, the file is now focused entirely on `RowGroupAccessPlanFilter`).
2706027 to
9071710Compareadriangb
commented
May 18, 2026
I'm splitting this into smaller PRs. |
adriangb
commented
May 18, 2026
Closing in favor of splitting this into three smaller, independently-reviewable PRs (per the request in the discussion to keep each move reviewable on its own):
Each is pure code motion, rebased on current One change from this PR: the |
…pache#22346) ## Which issue does this PR close? Relates to the discussion in apache#22024 about the Parquet datasource crate becoming hard to navigate. Split out of apache#22156, which bundled several code-motion moves into one PR — this is one of three smaller, independently-reviewable PRs that replace it. ## Rationale for this change `opener.rs` had grown to ~2,700 LOC, bundling several distinct responsibilities into one file. That makes it hard to read and hard to review changes in isolation. This PR is **pure code motion**: no behavior change and no public API change. ## What changes are included in this PR? Splits `opener.rs` into an `opener/` directory module: - `opener/early_stop.rs` — `EarlyStoppingStream`, the dynamic-filter early-termination wrapper applied at the end of `build_stream`. - `opener/encryption.rs` — `EncryptionContext` and the `ParquetMorselizer::get_encryption_context` helpers, isolating the `#[cfg(feature = "parquet_encryption")]` gating that previously bled through the main file. `opener.rs` becomes `opener/mod.rs`. Note: apache#22156 originally also extracted an `opener/push_decoder_stream.rs`. That move is now obsolete — apache#22289 has since extracted `PushDecoderStreamState` into `push_decoder.rs` — so it is dropped here. ## Are these changes tested? Yes, covered by existing tests. `cargo test -p datafusion-datasource-parquet --all-features` (122 passing) and `cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings` both pass. ## Are there any user-facing changes? No. `opener` was already a private module; this only reorganizes files inside the crate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ile_format.rs (apache#22347) ## Which issue does this PR close? Relates to the discussion in apache#22024 about the Parquet datasource crate becoming hard to navigate. Split out of apache#22156, which bundled several code-motion moves into one PR — this is one of three smaller, independently-reviewable PRs that replace it. ## Rationale for this change `file_format.rs` had grown to ~2,000 LOC, bundling several distinct responsibilities into one file. That makes it hard to read and hard to review changes in isolation. This PR is **pure code motion**: no behavior change and no public API change. ## What changes are included in this PR? Extracts two responsibilities from `file_format.rs` into focused modules (`file_format.rs` drops to ~660 LOC): - `sink.rs` — `ParquetSink` and the parallel-write machinery (`column_serializer_task`, `spawn_column_parallel_row_group_writer`, `output_single_parquet_file_parallelized`, `concatenate_parallel_row_groups`, etc.). - `schema_coercion.rs` — the Arrow-schema coercion utilities (`apply_file_schema_type_coercions`, `coerce_int96_to_resolution`, `coerce_file_schema_to_view_type`, `coerce_file_schema_to_string_type`, `transform_schema_to_view`, `transform_binary_to_string`, `field_with_new_type`) and their tests. Every previously-public item is still reachable at the same path: the crate root re-exports `sink::ParquetSink` and the `schema_coercion::*` functions, and the historical `file_format::ParquetSink` path is preserved via `pub use` (datafusion-proto depends on it). ## Are these changes tested? Yes, covered by existing tests (the `coerce_int96_to_resolution_*` tests moved with the function to `schema_coercion.rs`). `cargo test -p datafusion-datasource-parquet --all-features` (122 passing) and `cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings` both pass. `datafusion-proto` (a downstream `ParquetSink` consumer) builds clean. ## Are there any user-facing changes? No. Public API is unchanged — every previously-public item is still reachable at the same crate-root path. The only difference is the file organization inside the crate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ter.rs Pure code motion, no behavior change and no public API change. Extracts `BloomFilterStatistics` — the loaded Split Block Bloom Filter (SBBF) data plus its `PruningStatistics` adapter — from the ~1,900 LOC `row_group_filter.rs` into a new `bloom_filter.rs`. This separates "data we loaded from the file" (`BloomFilterStatistics`) from "the access-plan filter that consumes it" (`RowGroupAccessPlanFilter`), leaving `row_group_filter.rs` focused on the latter. `BloomFilterStatistics` is crate-internal; `row_group_filter` re-exports it (`pub(crate) use`) so the existing `crate::row_group_filter::BloomFilterStatistics` path keeps resolving for in-crate callers and this PR touches no other file. Split out of apache#22156. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ter.rs (apache#22348) ## Which issue does this PR close? Relates to the discussion in apache#22024 about the Parquet datasource crate becoming hard to navigate. Split out of apache#22156, which bundled several code-motion moves into one PR — this is one of three smaller, independently-reviewable PRs that replace it. ## Rationale for this change `row_group_filter.rs` had grown to ~1,900 LOC. It mixes "data we loaded from the file" with "the access-plan filter that consumes it." This PR is **code motion only**: no behavior change and no public API change. ## What changes are included in this PR? Extracts `BloomFilterStatistics` — the loaded Split Block Bloom Filter (SBBF) data plus its `PruningStatistics` adapter — from `row_group_filter.rs` into a new `bloom_filter.rs`, and moves the bloom-filter tests alongside it. This separates `BloomFilterStatistics` (data loaded from the file) from `RowGroupAccessPlanFilter` (the access-plan filter that consumes it), leaving `row_group_filter.rs` focused on the latter. Each commit builds green on its own: 1. **Split `bloom_filter` out of `row_group_filter.rs`** — move `BloomFilterStatistics` and its `PruningStatistics` adapter into a new `bloom_filter.rs`. 2. **Build `BloomFilterStatistics` via its constructors in tests** — the test built it with a struct literal; use the existing `with_capacity`/`insert` constructors (the pattern `opener.rs` already uses) so the `column_sbbf` field stays private. 3. **Extract `ExpectedPruning` into a shared `test_util` module** — the one test helper shared between the row-group and bloom-filter tests, so the bloom-filter tests can move out. Adds a `#[cfg(test)] pub(crate) fn access_plan()` accessor on `RowGroupAccessPlanFilter` so the helper can assert from a sibling module without widening field visibility. 4. **Move the bloom-filter tests into `bloom_filter.rs`** — relocate `test_row_group_bloom_*`, the `BloomFilterTest` builder, and its helper next to the code they exercise. `BloomFilterStatistics` is crate-internal; `row_group_filter` re-exports it (`pub(crate) use`) so the existing `crate::row_group_filter::BloomFilterStatistics` path keeps resolving for in-crate callers. Aside from `row_group_filter.rs` and `bloom_filter.rs`, this adds a new `src/test_util.rs` and a one-line module declaration in `mod.rs`. ## Are these changes tested? Yes, covered by existing tests. `cargo test -p datafusion-datasource-parquet --all-features` (122 passing) and `cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings` both pass. ## Are there any user-facing changes? No. `BloomFilterStatistics` is crate-internal; this only reorganizes files inside the crate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alamb
commented
May 20, 2026
And by the time I found this they are all merged 🎉 |
Which issue does this PR close?
Relates to the discussion in #22024 about the Parquet datasource crate becoming hard to navigate as more features land. Does not close that issue — this is a small, mechanical step that makes future hook/extension work easier.
Rationale for this change
Several files in the parquet datasource crate have grown to bundle multiple responsibilities into one place — making it hard to read and review changes in isolation. This PR is pure code motion: no public API changes (existing import paths preserved via re-exports), no behavior changes.
The hook/extension trait that addresses #22024's broader concern ("the opener is becoming a mini planner") is being prepared as a separate PR so reviewers can evaluate each change on its own merits.
What changes are included in this PR?
Five separate moves grouped into one PR because they're all the same flavor of work:
1.
opener.rs(~2,700 LOC) →opener/moduleopener/early_stop.rs—EarlyStoppingStream(~107 LOC), the dynamic-filter early-termination wrapper used at the end ofbuild_stream.opener/encryption.rs—EncryptionContext+ theParquetMorselizer::get_encryption_contexthelpers (~104 LOC). Isolates the#[cfg(feature = "parquet_encryption")]gating that previously bled through the main file.opener/push_decoder_stream.rs—PushDecoderStreamState(~135 LOC), the inner stream state machine that drives aParquetPushDecoder. Distinct from the outer (open-file) state machine they previously shared a file with.2.
file_format.rs(~2,000 LOC) splitsink.rs—ParquetSink+ the parallel-write machinery (column_serializer_task,spawn_column_parallel_row_group_writer,output_single_parquet_file_parallelized,concatenate_parallel_row_groups, etc.). The historicalfile_format::ParquetSinkpath is preserved viapub use.schema_coercion.rs— Arrow-schema coercion utilities (apply_file_schema_type_coercions,coerce_int96_to_resolution,coerce_file_schema_to_view_type,coerce_file_schema_to_string_type,transform_schema_to_view,transform_binary_to_string,field_with_new_type) and their tests. Re-exported at the crate root for backward compat.3.
row_group_filter.rs(~1,900 LOC)bloom_filter.rs—BloomFilterStatistics(the loaded SBBF data + itsPruningStatisticsadapter). Separates "data we loaded from the file" from "the access-plan filter that consumes it."Net effect
opener.rs(nowopener/mod.rs)file_format.rsrow_group_filter.rsAre these changes tested?
Yes:
datafusion-datasource-parquetunit tests pass (thecoerce_int96_to_resolution_*tests moved with the function toschema_coercion.rs).cargo fmt --all,./dev/rust_lint.sh,cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warningsall pass.datafusioncore builds clean.datafusion-proto(which importsParquetSinkvia the historicalfile_format::ParquetSinkpath) builds clean thanks to thepub usere-export.Are there any user-facing changes?
No. Public API is unchanged — every previously-public item is still reachable at the same crate-root path. The only difference is the file organization inside the crate.