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
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @brancz -- I think this looks really good
I have been thinking about how to remove the dependency sooner than 2 major releases (6 months)
One idea I had was that as a follow on PR, we can introduce a new feature flag (that was not enabled by default) like "no_object_store" that avoids the dependence
That way downstream systems that want to remove the dependence can do so (and lose the API) but would have to "opt in" for a few releases
| /// 1. Writes a Parquet file to an [`ObjectStore`] by passing an | ||
| /// [`object_store::buffered::BufWriter`] directly to [`AsyncArrowWriter`], | ||
| /// via the blanket [`AsyncFileWriter`] implementation for types | ||
| /// implementing [`AsyncWrite`] (replaces `ParquetObjectWriter`). |
There was a problem hiding this comment.
not: maybe we could say "equivalent of ParquetObjectWriter" rather than "replaces"
| /// implementing [`AsyncWrite`] (replaces `ParquetObjectWriter`). | ||
| /// | ||
| /// 2. Reads it back with [`ObjectStoreReader`], a minimal [`AsyncFileReader`] | ||
| /// implementation on top of an [`ObjectStore`] (replaces |
There was a problem hiding this comment.
similarly here "equivalent of " ...
| /// | ||
| /// 3. Reads it again with the reader wrapped in a [`SpawnedReader`], which | ||
| /// performs all I/O on a separate tokio runtime so that the runtime | ||
| /// decoding Parquet is not also driving the I/O (replaces |
| /// Note: this method does not make any effort to combine consecutive ranges, nor coalesce | ||
| /// ranges that are close together. This is instead delegated to the IO subsystem to optimise, | ||
| /// e.g. [`ObjectStore::get_ranges`](object_store::ObjectStore::get_ranges) | ||
| /// e.g. `ObjectStore::get_ranges` in the `object_store` crate |
There was a problem hiding this comment.
perhaps we can link to https://crates.io/crates/object_store
| /// 2. Implementations for remote storage, such as the `object_store` crate, | ||
| /// can implement this interface directly, typically by pairing a store | ||
| /// handle with an object path and delegating [`Self::get_bytes`] and | ||
| /// [`Self::get_byte_ranges`] to ranged reads. [`SpawnedReader`] can wrap |
| /// [`AsyncArrowWriter`]: crate::arrow::async_writer::AsyncArrowWriter | ||
| #[deprecated( | ||
| since = "59.2.0", | ||
| note = "Pass an `object_store::buffered::BufWriter` to `AsyncArrowWriter` directly instead; see `parquet/examples/object_store.rs`" |
There was a problem hiding this comment.
I double checked that this is the right suggestion
https://docs.rs/object_store/latest/object_store/buffered/struct.BufWriter.html
# Conflicts: # parquet/src/arrow/arrow_reader/selection.rs
|
Fixed the comments and merged main into this (let me know if you prefer a rebase). Should be ready to go. |
|
Pushing new commits is preferred (everything is squashed merged to main). I'll plan to merge it tomorrow. I won't cut the release without it |
|
Happy to see this change (wearing an opendal hat 🤩) I would happy to submit an opendal example as a follow up. |
|
FWIW an alternative way to resolve the actual issue, which appears to be wanting newer versions of object_store in downstreams faster, is to have versioned feature flags, i.e. AFAICT the proposed change here would really just move this upgrade hazard into the next layer in the chain, i.e. DataFusion |
|
FWIW I had Claude vomit out #10454 as a POC |
Yes I believe this is an accurate assesment. |
|
In my opinion, it is hard to justify why the parquet crate has a built in integration for one I/O system but not others (OpenDAL being an obvious example, but there are others). The argument that earlier on we needed an integration to help inform API design is reasonable, but I think that time has past now I personally think separating out the I/O concerns from the lower level decoder logic will make the parquet reader easier to use and customize downstream systems (e.g. to do prefetching of I/O for example). There are also projects that don't use object_store as their I/O layer so cutting the dependency I think is the right long term architectural direction. |
This is an interesting approach, and I am not opposed to it, but I still think moving the object store integration out of the parquet crate is the right technical approach If we are concerned with the effect on downstream migrations, we could potentially moving the integration to a new crate ( However, given the amount of code it would have (10s of lines of integration), a new crate seemed unecessary. However, if others feel strongly I would support that. We would still hvae to deprecate the export in this crate before removing them, so I think this PR would still be necessary |
|
I personally don't think it is all that hard to justify that we provide a batteries included option using the IO abstraction maintained as part of the arrow project, and have APIs to allow plugging in other IO abstractions should users wish to use them. That being said I was mainly responding to the linked ticket, as if the issue is upgrade hazards, I fail to see that this PR actually is an improvement in that regard - if anything it will make it worse as each downstream project is forced to adopt their own variant of it. That being said I don't feel overly strongly in this regard, and don't know what other plans are in the works, was just responding as I was tagged 😅 |
Given this feedback the more I am liking the approach of moving the object_store code to a new crate -- this would mean we still provide a first party integration but it would be an opt-in dependency 🤔 |
|
I don't really understand what batteries-included really means for an in-memory format like arrow. I get batteries-included (but removable) as a philosophy for datafusion. I see all of this work in the realm of "but removable", not meaning worse for downstream users, but more flexibility for downstream users (eg. opendal for reads, vortex as file format), and given that datafusion has become somewhat of a framework to build query engines, I think that's beneficial to fuel this way the community has picket it up. All that said, a first-party crate is totally reasonable, and with these changes, that crate would be pretty tiny and an easy follow-up. We should just be careful not to create the same coupling. |
|
If we are going to go with a first party crate I think we should probably do that as part of deprecation(so we don't tell users we are goign to remove the feature, rather we tell them they need to add a new crate dependency) I can make a PR to do that , but I am pretty busy the next day or two -- if someone else could do that (basically move the code to a new crate, and update the deprecation message to say "use the version in parquet-object-store" or whatever the crate is known |
|
We could also publish the deprecation warning and see if any downstream crates complain 🤔 And if there is demand then we can make the crate |
|
Would this crate be versioned in the same way as the arrow crates, or have it's own versioning, or do some feature flag based versioning?
I agree for an in-memory format it doesn't make much sense to bundle an IO abstraction, however, here we are talking about file formats - parquet / avro. Providing a way to read these formats from where they are most often stored, is pretty table stakes IMO. |
TBC I don't see any issues with this this approach, my point is more that it doesn't really resolve the linked issue, rather just punts it downstream 😅 Maybe downstreams make more frequent breaking releases and therefore this is less painful there, idk, I'm just struggling to understand the vision here |
|
I don't think punting it to downstream is a negative thing, nor does it not solve the issue. It means downstream can be flexible about it. That's not to say that downstream is as flexible yet as I think it should be, but that'll be follow-up work I'm intending to work on anyway. Not having the hard dependency allows downstream users to control their destiny, including, but not limited to, the version bump in question. I think something like a parquet-object-store crate should be separately versioned from arrow, just like |
This is fair -- what a new crate might be doing is easing the migration for people who were happy with the current dependency chain. |
|
I plan to merge this tomorrow |
For reference, the OpenDAL community maintains a crate called |
4 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