Skip to content

fix(parquet): cache deferred page index loads - #24004

Draft
Minghan2005 wants to merge 1 commit into
apache:mainfrom
Minghan2005:codex/cache-deferred-page-index
Draft

fix(parquet): cache deferred page index loads#24004
Minghan2005 wants to merge 1 commit into
apache:mainfrom
Minghan2005:codex/cache-deferred-page-index

Conversation

@Minghan2005

Copy link
Copy Markdown

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 written
back to FileMetadataCache. Reopening the same file therefore repeated the
page-index request.

What changes are included in this PR?

  • Add a default no-op ParquetFileReaderFactory::cache_metadata hook so
    factories with a metadata cache can retain metadata augmented after the
    initial request without requiring changes from existing implementations.
  • Have CachedParquetFileReaderFactory replace the existing footer-only cache
    entry after a deferred page-index load. As with the existing metadata path,
    decrypted metadata is not cached.
  • Call the hook after the opener finishes loading the page index.
  • Add a regression test that verifies the cache entry gains the page index and
    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 -- --nocapture
  • cargo test -p datafusion-datasource-parquet --all-features test_deferred_page_index_is_cached -- --nocapture
  • cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings
  • cargo clippy -p datafusion-datasource-parquet --all-targets --all-features -- -D warnings
  • Full repository formatting, TOML formatting, license, spelling, Prettier,
    workflow-install, and rustdoc checks
  • cargo-semver-checks against upstream/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?

ParquetFileReaderFactory gains a default no-op cache-update hook. Existing
implementations 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.

@github-actions github-actions Bot added the datasource Changes to the datasource crate label Jul 30, 2026
@Minghan2005

Copy link
Copy Markdown
Author

@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.

@mbutrovich

Copy link
Copy Markdown
Contributor

@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-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.85%. Comparing base (62cfc0c) to head (f1ccee2).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

datasource Changes to the datasource crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deferred page-index load in the Parquet opener bypasses FileMetadataCache, causing repeated uncached I/O when the skip heuristic doesn't fire

3 participants