You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/07/31 20:20:10 UTC
[PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
alamb opened a new pull request, #24036:
URL: https://github.com/apache/datafusion/pull/24036
## Which issue does this PR close?
- Related to https://github.com/apache/datafusion/pull/24030
- Related to https://github.com/apache/arrow-rs/issues/9879
## Rationale for this change
Upstream arrow-rs is deprecating the ParquetObjectReader (see https://github.com/apache/arrow-rs/issues/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?
1. unify `ParquetFileReader` and `CachedParquetFileReader`
2. 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24036:
URL: https://github.com/apache/datafusion/pull/24036
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24036:
URL: https://github.com/apache/datafusion/pull/24036#discussion_r3693320320
##########
datafusion/datasource-parquet/src/reader.rs:
##########
@@ -293,7 +287,7 @@ impl AsyncFileReader for CachedParquetFileReader {
options: Option<&'a ArrowReaderOptions>,
) -> 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();
Review Comment:
this is now Option<Arc<..>> so it needs a clone (rather than being able to call Arc::clone directly
##########
datafusion/datasource-parquet/src/reader.rs:
##########
@@ -86,62 +86,6 @@ impl DefaultParquetFileReaderFactory {
}
}
-/// Implements [`AsyncFileReader`] for a parquet file in object storage.
Review Comment:
this was all duplicated with CachedParquetFilerReader and I have unified the two
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24036:
URL: https://github.com/apache/datafusion/pull/24036#discussion_r3693144343
##########
datafusion/datasource-parquet/src/bloom_filter.rs:
##########
@@ -644,17 +644,15 @@ mod tests {
let metrics = ExecutionPlanMetricsSet::new();
let file_metrics =
ParquetFileMetrics::new(0, object_meta.location.as_ref(), &metrics);
+ let store: Arc<dyn ObjectStore> = Arc::new(in_memory);
let inner =
- ParquetObjectReader::new(Arc::new(in_memory), object_meta.location.clone())
+ ParquetObjectReader::new(Arc::clone(&store), object_meta.location.clone())
.with_file_size(object_meta.size);
let partitioned_file = PartitionedFile::new_from_meta(object_meta);
- let reader = ParquetFileReader {
- inner,
- file_metrics: file_metrics.clone(),
- partitioned_file,
- };
+ let reader =
+ ParquetFileReader::new(file_metrics.clone(), store, inner, partitioned_file);
Review Comment:
`ParquetFileReader` now has a real `new()` method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24036:
URL: https://github.com/apache/datafusion/pull/24036#issuecomment-5147124006
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-datasource-parquet v54.1.0 (current)
Built [ 50.346s] (current)
Parsing datafusion-datasource-parquet v54.1.0 (current)
Parsed [ 0.032s] (current)
Building datafusion-datasource-parquet v54.1.0 (baseline)
Built [ 45.440s] (baseline)
Parsing datafusion-datasource-parquet v54.1.0 (baseline)
Parsed [ 0.031s] (baseline)
Checking datafusion-datasource-parquet v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.147s] 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/e1a21a4874346164a6142dda64cc03399b2e6fdf/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/e1a21a4874346164a6142dda64cc03399b2e6fdf/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/e1a21a4874346164a6142dda64cc03399b2e6fdf/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/e1a21a4874346164a6142dda64cc03399b2e6fdf/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 [ 97.530s] datafusion-datasource-parquet
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24036:
URL: https://github.com/apache/datafusion/pull/24036#issuecomment-5147503072
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24036?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.85714%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.85%. Comparing base ([`dbcb5c0`](https://app.codecov.io/gh/apache/datafusion/commit/dbcb5c0f729e9ef6b0ab4c79253fe3b657929f48?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`faa8de8`](https://app.codecov.io/gh/apache/datafusion/commit/faa8de88c9b721edfcce84486e9a80d32dd53d55?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24036?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/datasource-parquet/src/reader.rs](https://app.codecov.io/gh/apache/datafusion/pull/24036?src=pr&el=tree&filepath=datafusion%2Fdatasource-parquet%2Fsrc%2Freader.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlLXBhcnF1ZXQvc3JjL3JlYWRlci5ycw==) | 80.64% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24036 +/- ##
==========================================
- Coverage 80.85% 80.85% -0.01%
==========================================
Files 1101 1101
Lines 374933 374916 -17
Branches 374933 374916 -17
==========================================
- Hits 303166 303144 -22
- Misses 53671 53673 +2
- Partials 18096 18099 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24036?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: unify `ParquetFileReader` and `CachedParquetFileReader` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24036:
URL: https://github.com/apache/datafusion/pull/24036#issuecomment-5183864716
Thank you for the review @zhuqi-lucas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org