Repository navigation
fix(parquet): cache deferred page index loads - #24004
Minghan2005 wants to merge 1 commit into
Conversation
|
@mbutrovich, could you please approve the workflows for this first-time contribution when convenient? All local checks listed in the PR description pass. I am keeping the PR in draft while CI runs. |
Running. Thanks for the PR! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24004 +/- ##
========================================
Coverage 80.85% 80.85%
========================================
Files 1096 1096
Lines 373911 374017 +106
Branches 373911 374017 +106
========================================
+ Hits 302329 302416 +87
- Misses 53547 53559 +12
- Partials 18035 18042 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
Rationale for this change
The Parquet opener intentionally loads footer-only metadata first so row-group
statistics can decide whether page-index I/O is necessary. When the page index
is needed later, however, the deferred byte-range load bypasses
DFParquetMetadata::fetch_metadata, so the augmented metadata was not writtenback to
FileMetadataCache. Reopening the same file therefore repeated thepage-index request.
What changes are included in this PR?
ParquetFileReaderFactory::cache_metadatahook sofactories with a metadata cache can retain metadata augmented after the
initial request without requiring changes from existing implementations.
CachedParquetFileReaderFactoryreplace the existing footer-only cacheentry after a deferred page-index load. As with the existing metadata path,
decrypted metadata is not cached.
that a second open performs zero reader byte-range I/O.
Are these changes tested?
Yes.
cargo test -p datafusion-datasource-parquet(159 passed)cargo test -p datafusion-datasource-parquet page_index -- --nocapturecargo test -p datafusion-datasource-parquet --all-features test_deferred_page_index_is_cached -- --nocapturecargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warningscargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warningsworkflow-install, and rustdoc checks
cargo-semver-checksagainstupstream/main(223 checks passed)The regression test measures the I/O invariant directly rather than relying on
wall-clock timing, so no separate benchmark was added.
Are there any user-facing changes?
ParquetFileReaderFactorygains a default no-op cache-update hook. Existingimplementations remain source-compatible, and the SemVer check reports that no
version update is required. The default cached reader now avoids repeated
page-index requests when opening the same file.
AI assistance
I used OpenAI Codex while implementing this change. I reviewed the code and
tests and understand the metadata-loading and cache-update path end to end.