-
Notifications
You must be signed in to change notification settings - Fork 861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document Async decoder usage (#4043) (#78) #4046
Conversation
arrow-csv/src/reader/mod.rs
Outdated
//! | ||
//! # Async Usage | ||
//! | ||
//! The lower-level [`Decoder`] can be integrated with various forms of async data streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "various forms" here is key, one of the major challenges in accommodating the async ecosystem is there is no one-size fits all async IO primitive, the intention behind the Decoder interface is to not be opinionated about where the bytes are coming from. This is similar to the AsyncFileReader trait we have for parquet, which similarly is not opinionated about what the underlying IO primitive actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information should be part of the doc-string, not hidden in some PR comment.
250d95b
to
caa43fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits. Thanks for the nice code examples, they help a lot!
arrow-csv/src/reader/mod.rs
Outdated
//! | ||
//! # Async Usage | ||
//! | ||
//! The lower-level [`Decoder`] can be integrated with various forms of async data streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information should be part of the doc-string, not hidden in some PR comment.
arrow-csv/src/reader/mod.rs
Outdated
//! // Note: the decoder needs to be called with an empty | ||
//! // array to delimit the final record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this done in this example? I would have expected that there's some decoder.finish()
method instead of some sentinel value for this purpose (I've seen a similar API design w/ some compressor crates before).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallthrough of the branch above will end up here, I'll move it up to the match on poll_next_unpin to make it more clear
Which issue does this PR close?
Closes #4043
Closes #78
Rationale for this change
It isn't immediately obvious how to use these decoders in an async setting, so lets document it.
What changes are included in this PR?
Are there any user-facing changes?