refactor: share leaf-level parquet read plan assembly with the row filter#23427
Closed
adriangb wants to merge 2 commits into
Closed
refactor: share leaf-level parquet read plan assembly with the row filter#23427adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
`build_projection_read_plan` had three copies of the same `ProjectionMask::roots` + `Schema::project` + `ParquetReadPlan` sequence, one per early-return path. Extract `root_level_plan`. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BsRGsN18YBCji5iFTQmWpr
`build_projection_read_plan` and `row_filter::build_parquet_read_plan` ran the same chain to build a leaf-level plan: leaf_indices_for_roots, resolve_struct_field_leaves, sort/dedup, ProjectionMask::leaves, build_filter_schema. They differed only in the row filter also sizing the leaves and returning an Option. Extract `assemble_read_plan`, which returns the plan plus the resolved leaf indices so the row filter can compute `required_bytes` from them. The three helpers it subsumes are now private to the module. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BsRGsN18YBCji5iFTQmWpr
adriangb
force-pushed
the
projection-read-plan-assemble
branch
from
e9db8a1 to
636a115
Compare
Contributor
Author
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
1 participant
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?
Stacked on #23426. Please merge that one first; until it lands this PR's diff also contains its commit. The change reviewable here is the last commit,
assemble_read_plan.Rationale for this change
projection_read_plan::build_projection_read_planandrow_filter::build_parquet_read_planboth end in the same chain to build a leaf-level plan:They differ only in that the row filter also sizes the resulting leaves (
required_bytes) and returns anOption. As @mbutrovich noted in review, a shared helper collapses both.What changes are included in this PR?
assemble_read_plan(root_indices, struct_field_accesses, file_schema, schema_descr)inprojection_read_plan.rs. It returns theParquetReadPlanplus the resolved leaf indices, which is what lets the row filter keep computingrequired_byteswithout duplicating the leaf resolution.build_parquet_read_planshrinks from ~30 lines to ~10.leaf_indices_for_roots,resolve_struct_field_leavesandbuild_filter_schemaare now private toprojection_read_planrather thanpub(crate), sinceassemble_read_planis the only remaining caller.No behavior change.
Are these changes tested?
Yes, by existing tests:
datafusion-datasource-parquetunit tests (158 pass) and theparquet_integrationsuite indatafusion(213 pass), both unchanged. The row-filter path is covered byparquet::filter_pushdownand the struct-field pushdown tests inrow_filter.rs.Are there any user-facing changes?
No.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BsRGsN18YBCji5iFTQmWpr