parquet: Remove explicit object_store integration#10354
Conversation
alamb
changed the title
object_store integration
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @brancz -- I think this is the right approach and I think it will make using the parquet reader with many downstream crates much easier
My only concern with this PR is now how to minimize the downstream impact / make upgrading as simple for people as possible. To that end, I think we need:
- A step-by-step upgrade guide (that can be mechanically followed)
- A way to get the old behavior (including with_runtime, etc)
What I was thinking is that we can move (literally move) the code from the existing reader/writer into a file in the examples directory, and then update the comments explaining the rationale / how to upgrade.
I have some ideas on how to do this. I'll make a PR shortly for your consideration
| .head(&Path::from("uniform_encryption.parquet.encrypted")) | ||
| .await | ||
| .unwrap(); | ||
| fn to_parquet_err(e: object_store::Error) -> ParquetError { |
There was a problem hiding this comment.
This is a pretty compelling short example showing the integration doesn't take much code (150 lines or so)
|
Is the reason this doesn't close #10308 is that it doesn't address the avro reader? |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
The more I thought about upgrade, the more I think if we just remove the object_store stuff in 60 that will be a jarring upgrade
Instead, I think we should do this in two steps (PRS) to minimize the blast radius:
- Deprecate the ParquetObjectReader and ParquetObjectWriter and point them at a new example (
parquet/examples/object_store.rs) that shows how to inline the structures in downstream crates - Actually remove the
ParquetObjectReaderandParquetObjectWriter
|
Here is what I had in mind |
|
Also, according to the https://github.com/apache/arrow-rs#deprecation-guidelines we should wait for 2 major versions before removing so I think we should begin deprecating first |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think we need to be a little more gentle when removing APIs -- per the https://github.com/apache/arrow-rs#deprecation-guidelines we should wait for 2 major versions before removing so I think we should begin deprecating first
89e20e6 to
424dae5
Compare
|
Alright, changed it to just deprecate, but kept the added helpers so the replacement code can be only ~60 lines as opposed to ~400 without them (see the example). If this is generally what we want the avro deprecations to look like as well, I can prepare those changes as well. |
Deprecate ParquetObjectReader/ParquetObjectWriter in favor of implementing AsyncFileReader directly (with an example on the trait docs) and passing an AsyncWrite such as object_store's BufWriter to AsyncArrowWriter. To reduce the code needed by implementors I also added SpawnedReader and ParquetMetaDataReader::with_arrow_reader_options. In the future we will only need to keep object_store as a dev-dependency to we can ensure compatibility and use it in benchmarking but remove it from being a full runtime dependency all together. Also added a full example in `parquet/examples/object_store.rs`.
424dae5 to
160a16d
Compare
2 participants
Deprecate ParquetObjectReader/ParquetObjectWriter in favor of
implementing AsyncFileReader directly (with an example on the trait
docs) and passing an AsyncWrite such as object_store's BufWriter to
AsyncArrowWriter. To reduce the code needed by implementors I also added
SpawnedReader and ParquetMetaDataReader::with_arrow_reader_options. In
the future we will only need to keep object_store as a dev-dependency to
we can ensure compatibility and use it in benchmarking but remove it
from being a full runtime dependency all together.
Also added a full example in
parquet/examples/object_store.rs.Which issue does this PR close?
If we agree on the approach on this, I basically already have the same thing ready for avro, but wanted to keep the PR smaller so we can align on the direction before doing it all at once.
What changes are included in this PR?
Deprecate the object_store dependency and add some extra helpers to allow users to replace it with very few lines of code (see the example where it only takes ~60 lines to replace what would take hundreds without the helpers).
Are these changes tested?
Yes
Are there any user-facing changes?
Not yet, just deprecations.
Additional notes
Full disclaimer: Since we don't use parquet, I don't know this code super well, so I used Claude Code a bit to help me in creating this patch. That said, I've fully read the AI-generated parts and understand what they do, and they are how I would have written it, and how I interpreted the prior discussion in #10308.
@alamb