feat: Implement an AsyncReader for avro using ObjectStore#8930
feat: Implement an AsyncReader for avro using ObjectStore#8930alamb merged 40 commits intoapache:mainfrom
Conversation
github-actions
bot
added
parquet
jecsand838
left a comment
jecsand838
left a comment
There was a problem hiding this comment.
Flushing a partial review with some high level thoughts.
I'll wait for you to finish before resuming.
Honestly I think my main blocker is the schema thing here. I don't want to commit to the constructor before it is resolved as its a public API and I don't want it to be volatile |
100% I'm working on that right now and won't stop until I have a PR. That was a solid catch. The schema logic is an area of the code I mean to (or would welcome) a full refactor of. I knew it would eventually come back. |
|
Sorry, I haven't dropped it, just found myself in a really busy week! The generic reader support does not seem to hard to implement from the dabbling I made, and I still need to get to the builder pattern change |
…, separate object store file reader into a featuregated struct and use a generic async file reader trait
|
@jecsand838 I believe this is now ready for a proper review^ |
There was a problem hiding this comment.
@EmilyMatt Thank you so much for getting these changes up!
I left a few comments. Let me know what you think.
EDIT: Should have mentioned that this is looking really good overall and I'm very excited for the AsyncReader!
|
@jecsand838 and @EmilyMatt -- how is this PR looking? |
I had actually just returned to work on it 2 days ago, still having some issues with the schema now being provided, due to the problems I've described, @jecsand838 suggested removing the arrow schema and I'm starting to think that is the only viable way for now. |
|
Hope to push another version today and address some of the things above |
|
@jecsand838 I've removed the parquet changes, and synced with main, I believe this is ready for last reviews and test runs before merged. CC: @alamb |
|
I started the tests |
Thx, most failures were technicalities, believe I fixed all of them |
jecsand838
left a comment
jecsand838
left a comment
There was a problem hiding this comment.
@EmilyMatt LGTM!
At this point I'm fine with anything else that comes up being a follow-up issue if you are @mzabaluev (unless it's major).
I just left a few final comments related to improving the docs for this PR.
| //! is enabled, [`AvroObjectReader`] provides integration with object storage services | ||
| //! such as S3 via the [object_store] crate. | ||
| //! | ||
| //! ```ignore |
There was a problem hiding this comment.
Let's make this runnable
| //! ```ignore | |
| //! ``` |
There was a problem hiding this comment.
I don't know how to do it without failing the tests, because all the code here is featuregated, and the doctests also runs without features enabled
# Conflicts: # arrow-avro/Cargo.toml # arrow-avro/src/reader/mod.rs
mzabaluev
left a comment
mzabaluev
left a comment
There was a problem hiding this comment.
Looks good and works well.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I took a look at this and it looks fine to me -- I mostly deferring the reviews content to @jecsand838 and @mzabaluev and focused on the API design
Thanks @jecsand838 and @mzabaluev for the review
|
Are there any further blockers? This is ready on my end. |
LGTM! Thank you for all of this @EmilyMatt. Really excited for the |
|
Let's gogogogogogogo |
|
Thank you @EmilyMatt @jecsand838 and @mzabaluev -- epic |
5 participants
Which issue does this PR close?
Rationale for this change
Allows for proper file splitting within an asynchronous context.
What changes are included in this PR?
The raw implementation, allowing for file splitting, starting mid-block(read until sync marker is found), and further reading until end of block is found.
This reader currently requires a reader_schema is provided if type-promotion, schema-evolution, or projection are desired.
This is done so because #8928 is currently blocking proper parsing from an ArrowSchema
Are these changes tested?
Yes
Are there any user-facing changes?
Only the addition.
Other changes are internal to the crate (namely the way Decoder is created from parts)