refactor: unify `ParquetFileReader` and `CachedParquetFileReader` by alamb · Pull Request #24036 · apache/datafusion

alamb · 2026-07-31T20:20:01Z

Which issue does this PR close?

Related to chore(deps): Update to arrow/parquet 59.2.0 #24030
Related to Release arrow-rs / parquet Minor version 59.2.0 (July 2026) arrow-rs#9879

Rationale for this change

Upstream arrow-rs is deprecating the ParquetObjectReader (see apache/arrow-rs#10308)

Not using the deprecated code involves copying some code for each existing usage, and I found that CachedParquetFileReader and ParquetFileReader are almost the same

What changes are included in this PR?

unify ParquetFileReader and CachedParquetFileReader
make the fields non pub

Are these changes tested?

Yes by CI

Are there any user-facing changes?

There are breaking API changes, though I think for the better (things are now encapsulated more)

github-actions · 2026-07-31T20:22:42Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-datasource-parquet v54.1.0 (current)
       Built [  53.754s] (current)
     Parsing datafusion-datasource-parquet v54.1.0 (current)
      Parsed [   0.032s] (current)
    Building datafusion-datasource-parquet v54.1.0 (baseline)
       Built [  48.041s] (baseline)
     Parsing datafusion-datasource-parquet v54.1.0 (baseline)
      Parsed [   0.032s] (baseline)
    Checking datafusion-datasource-parquet v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.160s] 223 checks: 219 pass, 4 fail, 0 warn, 30 skip

--- failure constructible_struct_adds_private_field: struct no longer constructible due to new private field ---

Description:
A struct constructible with a struct literal has a new non-public field. It can no longer be constructed using a struct literal outside of its crate.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/constructible_struct_adds_private_field.ron

Failed in:
  field ParquetFileReader.store in /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:203
  field ParquetFileReader.metadata_cache in /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:206
  field ParquetFileReader.metadata_size_hint in /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:207

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/struct_missing.ron

Failed in:
  struct datafusion_datasource_parquet::CachedParquetFileReader, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/f74bc4c946e79e9f9ad1a780c035c680ab2c1b24/datafusion/datasource-parquet/src/reader.rs:240

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field file_metrics of struct ParquetFileReader, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/f74bc4c946e79e9f9ad1a780c035c680ab2c1b24/datafusion/datasource-parquet/src/reader.rs:98
  field inner of struct ParquetFileReader, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/f74bc4c946e79e9f9ad1a780c035c680ab2c1b24/datafusion/datasource-parquet/src/reader.rs:99
  field partitioned_file of struct ParquetFileReader, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/f74bc4c946e79e9f9ad1a780c035c680ab2c1b24/datafusion/datasource-parquet/src/reader.rs:100

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field ParquetFileReader.file_metrics in file /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:201
  field ParquetFileReader.inner in file /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:201
  field ParquetFileReader.partitioned_file in file /home/runner/work/datafusion/datafusion/datafusion/datasource-parquet/src/reader.rs:201

     Summary semver requires new major version: 4 major and 0 minor checks failed
    Finished [ 103.465s] datafusion-datasource-parquet

CachedParquetFileReader duplicated ParquetFileReader's get_bytes / get_byte_ranges implementations and differed only in whether it passed a FileMetadataCache to DFParquetMetadata when fetching metadata. Fold it into ParquetFileReader with an `Option<Arc<FileMetadataCache>>` field: `None` behaves like the old uncached reader (DefaultParquetFileReaderFactory), `Some(cache)` like the old cached one (CachedParquetFileReaderFactory). Also make all ParquetFileReader fields private and add file_metrics() and partitioned_file() accessors for the two that were previously public, narrowing the public surface now that construction goes through ParquetFileReaderFactory.

alamb · 2026-07-31T20:20:23Z

-            partitioned_file,
-        };
+        let reader =
+            ParquetFileReader::new(file_metrics.clone(), store, inner, partitioned_file);


ParquetFileReader now has a real new() method

alamb · 2026-07-31T20:56:00Z

    }
 }

-/// Implements [`AsyncFileReader`] for a parquet file in object storage.


this was all duplicated with CachedParquetFilerReader and I have unified the two

alamb · 2026-07-31T20:57:34Z

    ) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {
        let object_meta = self.partitioned_file.object_meta.clone();
-        let metadata_cache = Arc::clone(&self.metadata_cache);
+        let metadata_cache = self.metadata_cache.clone();


this is now Option<Arc<..>> so it needs a clone (rather than being able to call Arc::clone directly

codecov-commenter · 2026-07-31T21:11:31Z

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.89%. Comparing base (bc8b1a7) to head (f580cc6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/datasource-parquet/src/reader.rs	80.64%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24036      +/-   ##
==========================================
- Coverage   80.89%   80.89%   -0.01%     
==========================================
  Files        1102     1102              
  Lines      376196   376179      -17     
  Branches   376196   376179      -17     
==========================================
- Hits       304329   304301      -28     
- Misses      53756    53765       +9     
- Partials    18111    18113       +2

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

…ile-reader

alamb · 2026-08-04T19:50:08Z

Thank you for the review @zhuqi-lucas

…ile-reader

…24037) ## Which issue does this PR close? - Something I noticed while working on apache#24036 ## Rationale for this change `DFParquetMetadata` had several fields and methods with no doc comments, or only a one-line description, making it hard to understand their purpose without reading the implementation. ## What changes are included in this PR? Add doc comments ## Are these changes tested? No behavior changed, only doc comments were added; existing tests cover the code. ## Are there any user-facing changes? No functional changes; public API doc comments are improved.

alamb added the api change Changes the API exposed to users of the crate label Jul 31, 2026

github-actions Bot added the datasource Changes to the datasource crate label Jul 31, 2026

github-actions Bot added the auto detected api change Auto detected API change label Jul 31, 2026

alamb mentioned this pull request Jul 31, 2026

chore(deps): Update to arrow/parquet 59.2.0 #24030

Merged

alamb force-pushed the alamb/unify-parquet-file-reader branch from 0573892 to faa8de8 Compare July 31, 2026 20:44

github-actions Bot added the documentation Improvements or additions to documentation label Jul 31, 2026

alamb commented Jul 31, 2026

View reviewed changes

alamb marked this pull request as ready for review July 31, 2026 20:57

alamb mentioned this pull request Jul 31, 2026

docs: document all fields and methods of DFParquetMetadata #24037

Merged

zhuqi-lucas approved these changes Aug 3, 2026

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/unify-parquet-f…

67eb40b

…ile-reader

alamb enabled auto-merge August 4, 2026 19:50

alamb added 2 commits August 4, 2026 16:10

prettier

25b77f6

Merge remote-tracking branch 'apache/main' into alamb/unify-parquet-f…

f580cc6

…ile-reader

alamb added this pull request to the merge queue Aug 4, 2026

Merged via the queue into apache:main with commit e8015e4 Aug 4, 2026
38 checks passed

alamb deleted the alamb/unify-parquet-file-reader branch August 4, 2026 20:49

Navigation Menu

Uh oh!

refactor: unify `ParquetFileReader` and `CachedParquetFileReader` - #24036

refactor: unify `ParquetFileReader` and `CachedParquetFileReader`#24036
alamb merged 4 commits into
apache:mainfrom
alamb:alamb/unify-parquet-file-reader

alamb commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

alamb Jul 31, 2026

Uh oh!

alamb Jul 31, 2026

Uh oh!

alamb Jul 31, 2026

Uh oh!

codecov-commenter commented Jul 31, 2026 •

edited

Loading

Uh oh!

alamb commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

alamb commented Jul 31, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 31, 2026 •

edited

Loading

codecov-commenter commented Jul 31, 2026 •

edited

Loading