Skip to content
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

Merged
merged 3 commits into from Apr 11, 2023

Conversation

tustvold
Copy link
Contributor

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?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 10, 2023
//!
//! # Async Usage
//!
//! The lower-level [`Decoder`] can be integrated with various forms of async data streams.
Copy link
Contributor Author

@tustvold tustvold Apr 10, 2023

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.

Copy link
Contributor

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.

@tustvold tustvold mentioned this pull request Apr 10, 2023
Copy link
Contributor

@crepererum crepererum left a 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!

//!
//! # Async Usage
//!
//! The lower-level [`Decoder`] can be integrated with various forms of async data streams.
Copy link
Contributor

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.

Comment on lines 72 to 73
//! // Note: the decoder needs to be called with an empty
//! // array to delimit the final record
Copy link
Contributor

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).

Copy link
Contributor Author

@tustvold tustvold Apr 11, 2023

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

@tustvold tustvold merged commit 884ab4e into apache:master Apr 11, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async JSON reader Async CSV reader
2 participants