refactor: extract parquet projection read plan into its own module#23396
Open
adriangb wants to merge 2 commits into
Open
refactor: extract parquet projection read plan into its own module#23396adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
This was referenced Jul 8, 2026
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
force-pushed
the
extract-projection-read-plan
branch
from
69b2791 to
d4fb0c5
Compare
Contributor
Author
|
@mbutrovich I found this refactor useful while investigating apache/datafusion-comet#4859. |
xudong963
approved these changes
Jul 9, 2026
| use datafusion_physical_expr::expressions::Column; | ||
| use datafusion_physical_expr::utils::collect_columns; | ||
|
|
||
| use crate::row_filter::PushdownChecker; |
Member
There was a problem hiding this comment.
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
Comment on lines
+243
to
+249
| // 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"] |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
row_filter.rsis 2,100+ lines and contains two distinct concerns: row-filter/ArrowPredicateconstruction, 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_typeand the path-grouping helpers move fromrow_filter.rsto a newprojection_read_plan.rs, together with their test.PushdownChecker/PushdownColumnsbecomepub(crate)so the new module can keep using them.decoder_projection.rsimportsbuild_projection_read_planfrom 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-parquetand 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