refactor: extract parquet projection read plan into its own module by adriangb · Pull Request #23396 · apache/datafusion

adriangb · 2026-07-08T22:28:04Z

Which issue does this PR close?

N/A — pure code movement, split out of a larger change (see rationale). First of a 3-PR stack: this refactor, bench: parquet scan with a table schema narrower than a nested column #23397 (benchmark), and the feature PR (nested schema pruning for the parquet reader, upstream work for Comet native Parquet scan reads full nested columns despite pruned Spark ReadSchema datafusion-comet#4859).

Rationale for this change

row_filter.rs is 2,100+ lines and contains two distinct concerns: row-filter/ArrowPredicate construction, and the shared "expressions → parquet leaf indices → ProjectionMask + projected schema" resolution used by both the row filter and the opener's projection handling. This PR moves the second concern into its own module, projection_read_plan.rs, so upcoming changes to projection-mask derivation can be reviewed without wading through the row-filter machinery.

This is mergeable as standalone cleanup regardless of whether the follow-up feature is accepted.

What changes are included in this PR?

A move-only refactor (best reviewed with git diff --color-moved):

ParquetReadPlan, StructFieldAccess, build_projection_read_plan, leaf_indices_for_roots, resolve_struct_field_leaves, build_filter_schema, prune_struct_type and the path-grouping helpers move from row_filter.rs to a new projection_read_plan.rs, together with their test.
PushdownChecker / PushdownColumns become pub(crate) so the new module can keep using them.
decoder_projection.rs imports build_projection_read_plan from its new home.

No behavior change; net +57 lines (module docs, imports, visibility).

Are these changes tested?

Covered by existing tests (the moved unit test plus the datafusion-datasource-parquet and core parquet integration suites, which all pass unchanged).

Are there any user-facing changes?

No.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KuMaRtFSPDQesuzjN5Koyd

Move-only: `ParquetReadPlan`, `StructFieldAccess`, `build_projection_read_plan` and the leaf-index/schema-pruning helpers move from row_filter.rs (2100+ lines) into projection_read_plan.rs. `PushdownChecker`/`PushdownColumns` become pub(crate) so the new module can keep using them. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KuMaRtFSPDQesuzjN5Koyd

adriangb · 2026-07-08T22:49:01Z

@mbutrovich I found this refactor useful while investigating apache/datafusion-comet#4859. row_filter.rs is a beast so I think splitting it up makes sense regardless of other work. wdyt?

xudong963 · 2026-07-09T05:17:00Z

+use datafusion_physical_expr::expressions::Column;
+use datafusion_physical_expr::utils::collect_columns;
+
+use crate::row_filter::PushdownChecker;


projection_read_plan imports PushdownChecker from row_filter, whilerow_filter imports ParquetReadPlan and helpers from projection_read_plan.

How about moving PushdownChecker /PushdownColumns into projection_read_plan.rs or a neutral module like column_access.rs, then have row_filter.rs call into that

good call, moved in b34743b

xudong963 · 2026-07-09T05:17:49Z

+            // A leaf matches if its path starts with our prefix.
+            // e.g., prefix=["s", "value"] matches leaf path ["s", "value"]
+            // prefix=["s", "outer"] matches ["s", "outer", "inner"]
+
+            // a leaf matches if its path starts with our prefix
+            // for example: prefix=["s", "value"] matches leaf path ["s", "value"]
+            //              prefix=["s", "outer"] matches ["s", "outer", "inner"]


duplicated comments

Makes the imports between row_filter and projection_read_plan one-directional (row_filter -> projection_read_plan) instead of circular, and removes a duplicated comment in resolve_struct_field_leaves. Addresses review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016zX5Ga66kQPkscPVf7p1AN

adriangb · 2026-07-09T13:25:03Z

Planning on merging this later today since it's a pure refactor but could cause merge conflicts.

mbutrovich · 2026-07-09T14:54:08Z

I am reviewing this morning.

mbutrovich

Minor issues I found while reviewing (pre-existing) and suggestions for the follow-ups, I think. Thanks @adriangb!

mbutrovich · 2026-07-09T15:35:27Z

+    let leaf_indices = {
+        let mut out =
+            leaf_indices_for_roots(all_root_indices.iter().copied(), schema_descr);
+        let struct_leaf_indices =
+            resolve_struct_field_leaves(&all_struct_accesses, file_schema, schema_descr);
+
+        out.extend_from_slice(&struct_leaf_indices);
+        out.sort_unstable();
+        out.dedup();
+
+        out
+    };
+
+    let projection_mask =
+        ProjectionMask::leaves(schema_descr, leaf_indices.iter().copied());
+
+    let projected_schema =
+        build_filter_schema(file_schema, &all_root_indices, &all_struct_accesses);
+
+    ParquetReadPlan {
+        projection_mask,
+        projected_schema,
+    }


Not for this PR: this tail (L443-465) is nearly identical to build_parquet_read_plan in row_filter.rs (leaf_indices_for_roots → resolve_struct_field_leaves → extend/sort/dedup → ProjectionMask::leaves → build_filter_schema → ParquetReadPlan), differing only in required_bytes and the Option return. A shared assemble_read_plan(...) helper could collapse both in a follow-up.

mbutrovich · 2026-07-09T15:35:42Z

+        root_indices.sort_unstable();
+        root_indices.dedup();
+
+        let projection_mask =


Not for this PR: the ProjectionMask::roots + file_schema.project + build ParquetReadPlan sequence repeats three times (here plus L396-408 and L428-441). A small local helper (root_level_plan(&root_indices)) might help.

mbutrovich · 2026-07-09T15:36:17Z

+    // schema has no group columns (Struct, Map, etc.); when group columns
+    // exist, their children become separate leaves and shift all subsequent
+    // leaf indices.
+    // Struct columns are unsupported.


// Struct columns are unsupported. reads as an orphaned leftover here — leaf_indices_for_roots maps roots to leaves regardless of type, so the comment is misleading. Suggest deleting it or restating what it actually constrains.

mbutrovich · 2026-07-09T15:36:48Z

+                    && (!DataType::is_nested(return_type)
+                        || self.is_nested_type_supported(return_type))
+                {
+                    // try to resolve all field name arguments to strinrg literals


Typo: strinrg → string. Also a "what" comment where the code is self-evident; could drop it. (Pre-existing.)

mbutrovich · 2026-07-09T15:36:57Z

+            ),
+        );
+
+        // all3 Parquet leaves should be in the projection mask


Typo: all3 → all 3. (Pre-existing.)

Review feedback: `strinrg` typo, `all3` typo, and a leftover "Struct columns are unsupported" comment in `leaf_indices_for_roots` that no longer describes what the function does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BsRGsN18YBCji5iFTQmWpr

mbutrovich

Thanks @adriangb!

@mbutrovich

## Which issue does this PR close? - N/A — follow-up cleanup requested in review of apache#23396, in two comments: [`root_level_plan`](apache#23396 (comment)) and [`assemble_read_plan`](apache#23396 (comment)). Two independent commits, each green on its own; best reviewed commit by commit. ## Rationale for this change @mbutrovich spotted two pieces of duplication in `projection_read_plan.rs` while reviewing apache#23396, both deferred out of that (move-only) PR: 1. `build_projection_read_plan` has three early-return paths — the all-plain-columns fast path, the no-struct-columns fast path, and the no-struct-accesses fallback. Each repeated the same `ProjectionMask::roots` + `Schema::project` + construct-`ParquetReadPlan` sequence. 2. `build_projection_read_plan` and `row_filter::build_parquet_read_plan` both end in the same chain to build a leaf-level plan: ``` leaf_indices_for_roots → resolve_struct_field_leaves → extend/sort/dedup → ProjectionMask::leaves → build_filter_schema → ParquetReadPlan ``` They differ only in that the row filter also sizes the resulting leaves (`required_bytes`) and returns an `Option`. ## What changes are included in this PR? **Commit 1 — `root_level_plan`:** a new private helper taking sorted, deduplicated root indices and decoding every leaf below each root. The three early-return paths delegate to it. Net −9 lines. **Commit 2 — `assemble_read_plan`:** shared by both callers. It returns the `ParquetReadPlan` plus the resolved leaf indices, which is what lets the row filter keep computing `required_bytes` without duplicating leaf resolution. `build_parquet_read_plan` shrinks from ~30 lines to ~10. `leaf_indices_for_roots`, `resolve_struct_field_leaves` and `build_filter_schema` become private to `projection_read_plan`, since `assemble_read_plan` is now their only caller. No behavior change in either commit. ## Are these changes tested? Yes, by existing tests: `datafusion-datasource-parquet` unit tests (158 pass) and the `parquet_integration` suite in `datafusion` (213 pass), both unchanged and both green at each commit. The row-filter path touched by commit 2 is covered by `parquet::filter_pushdown` and the struct-field pushdown tests in `row_filter.rs`. ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BsRGsN18YBCji5iFTQmWpr --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…apache#23397) ## Which issue does this PR close? - Related to apache/datafusion-comet#4859. Second of a 3-PR stack: apache#23396 (refactor), this benchmark, and the feature PR (nested schema pruning for the parquet reader). ## Rationale for this change When a table's declared schema is narrower than a parquet file's nested column (logical `events: LIST<STRUCT<x, y>>` over a physical `LIST<STRUCT<x, y, +8 pads>>`), the reader currently fetches and decodes **every** leaf of the column and discards the extra subfields in memory via the adapter-inserted cast. This is how engines like Spark (via Comet) communicate nested projection pruning to the scan — as a clipped read schema — and it is where Comet measured reading 1.35 TB where Spark read 30.9 GB for the same pruned `ReadSchema`. This PR adds a benchmark that documents the current behavior as a checked-in baseline, independent of any fix: ``` list_struct_narrow_schema: bytes_scanned=25.19 MB 3.45 ms list_struct_full_schema: bytes_scanned=25.19 MB 3.36 ms <- narrow == full today list_struct_physically_narrow: bytes_scanned= 3.32 KB 164 µs <- the floor ``` ## What changes are included in this PR? A criterion benchmark, `datafusion/core/benches/parquet_nested_schema_pruning.rs`, that registers the same wide `list<struct>` (and top-level struct) parquet file with both its full schema and a narrower declared schema, plus a physically-narrow file as the floor, and prints the scans' `bytes_scanned` at setup so the IO pattern is visible alongside wall time. ## Are these changes tested? It is a benchmark; it compiles under `cargo bench --no-run` and runs green. ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KuMaRtFSPDQesuzjN5Koyd --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

github-actions Bot added the datasource Changes to the datasource crate label Jul 8, 2026

adriangb force-pushed the extract-projection-read-plan branch from 69b2791 to d4fb0c5 Compare July 8, 2026 22:36

xudong963 approved these changes Jul 9, 2026

View reviewed changes

mbutrovich reviewed Jul 9, 2026

View reviewed changes

mbutrovich approved these changes Jul 9, 2026

View reviewed changes

This was referenced Jul 9, 2026

refactor: de-duplicate parquet read plan construction #23426

Merged

refactor: share leaf-level parquet read plan assembly with the row filter #23427

Closed

adriangb enabled auto-merge July 9, 2026 16:43

adriangb added this pull request to the merge queue Jul 9, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 9, 2026

adriangb added this pull request to the merge queue Jul 9, 2026

Merged via the queue into apache:main with commit d213a8a Jul 9, 2026
36 checks passed

adriangb deleted the extract-projection-read-plan branch July 9, 2026 17:24

Uh oh!

Conversation

adriangb commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

adriangb commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Jul 9, 2026

Uh oh!

mbutrovich commented Jul 9, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adriangb commented Jul 8, 2026 •

edited

Loading