feat: Load Parquet page indexes for external row selections - #25460
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25460 +/- ##
==========================================
+ Coverage 82.38% 82.46% +0.07%
==========================================
Files 1138 1140 +2
Lines 434491 436818 +2327
Branches 434491 436818 +2327
==========================================
+ Hits 357969 360210 +2241
+ Misses 54875 54849 -26
- Partials 21647 21759 +112 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @haohuaijin , here are some suggestions
a3185dc to
f5f1b7f
Compare
|
Thanks for your reviews @jayzhan211 , i applied the suggestions in f5f1b7f |
|
run benchmark clickbench clickbench_partitioned |
alamb
left a comment
There was a problem hiding this comment.
Thanks @haohuaijin and @jayzhan211 -- I an a little worried about the overhead of this change as it seems to make an entire read plan and then discard it -- if it makes a read plan, shouldn't we be using that if possible?
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-row-selection-page-index (d104ad0) to d20936c (merge-base) diff Run configurationrun benchmark clickbenchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-row-selection-page-index (d104ad0) to d20936c (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/parquet-row-selection-page-index (d104ad0) to d20936c (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
run benchmark clickbench clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-row-selection-page-index (3f4da1b) to d20936c (merge-base) diff Run configurationrun benchmark clickbenchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-row-selection-page-index (3f4da1b) to d20936c (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/parquet-row-selection-page-index (3f4da1b) to d20936c (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
| struct RowGroupsPrunedParquetOpen { | ||
| prepared: FiltersPreparedParquetOpen, | ||
| row_groups: RowGroupAccessPlanFilter, | ||
| /// Built lazily for external-selection index checks and reused by the stream. |
| &prepared.output_schema, | ||
| prepared.virtual_state.as_deref(), | ||
| )?; | ||
| // Reuse plans built for the external-selection index check. Other |
| decoder_read_plans: None, | ||
| }; | ||
| open.should_load_page_index() | ||
| open.should_load_page_index().unwrap() |
There was a problem hiding this comment.
how do we know this won't panic? Maybe this should also return Result?
There was a problem hiding this comment.
this is a helper function in test mods, maybe it is ok to panic or should we change it to return a Result?
|
Thanks @haohuaijin and @alamb 🚀 |
Which issue does this PR close?
Rationale for this change
External row selections can benefit from offset indexes without a page-pruning predicate, or when row-group statistics fully match the scan predicate.
For example, an external index can select rows for a service while the Parquet scan checks only a time range. A row group entirely within that range is fully matched for the predicate, but the external selection can still exclude most rows.
Offset indexes allow the reader to locate relevant pages by row position. Column-index statistics are unnecessary for this path because the selection already identifies the rows to read.
What changes are included in this PR?
enable_page_indexbefore either loading path.The projection check controls whether loading is triggered; it does not restrict index I/O to projected columns. Read-plan caching, decoder refactoring, and arrow-rs changes are outside this PR.
What is the testing strategy for this PR?
Unit tests cover offset-only metadata, missing indexes, fully matched and skipped row groups, selector/bitmap representations, empty/uniform selections, and indexes only on unprojected columns. They also verify the predicate-based fallback.
A V1 Parquet test selects the final 100 of 10,000 rows and checks exact output values and reduced
bytes_scanned, both without a predicate and with a fully matching predicate.The following checks passed, including all 11 page-index tests:
cargo test -p datafusion-datasource-parquet --lib page_index cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings cargo fmt --all -- --checkThe repository-wide
./dev/rust_lint.shcould not proceed because the local Python environment is missing PyYAML. The targeted checks above passed.Are there any user-facing changes?
External partial row selections can use offset indexes to skip unselected pages without a useful page-pruning predicate. Query results and public APIs are unchanged, and disabling page indexes still disables this loading path.