-
Notifications
You must be signed in to change notification settings - Fork 853
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
Minor: Improve docs for arrow-ipc, remove clippy ignore #3421
Conversation
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.
cc @Dandandan as I believe you have an interest in this
/// | ||
/// // Error of dictionary ids are replaced. | ||
/// let error_on_replacement = true; | ||
/// let options = IpcWriteOptions::default(); |
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 interface is somewhat unfortunate (like there is no state on IpcDataGenerator and the state is passed in via DictionaryTracker).
While implementing #3389 I hope to provide something that handles all the encoding state so this interface will remain low level
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.
I wonder if the state was at one point on IpcDataGenerator and it got factored out for some reason 🤔
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.
🤷
/// | ||
/// // Error of dictionary ids are replaced. | ||
/// let error_on_replacement = true; | ||
/// let options = IpcWriteOptions::default(); |
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.
I wonder if the state was at one point on IpcDataGenerator and it got factored out for some reason 🤔
arrow-ipc/src/lib.rs
Outdated
|
||
// TODO: (vcq): Protobuf codegen is not generating Debug impls. | ||
#![allow(missing_debug_implementations)] | ||
//! Support for the [Arrow IPC Format](https://arrow.apache.org/docs/format/IPC.html) |
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.
Actually this page was moved. Maybe we can point to https://arrow.apache.org/docs/format/Columnar.html#format-columnar, or https://arrow.apache.org/docs/format/Columnar.html#serialization-and-interprocess-communication-ipc specifically?
Benchmark runs are scheduled for baseline = ec43d6f and contender = 2408bb2. 2408bb2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
N/A
Rationale for this change
A deprecation warning says to use
IpcDataGenerator
but then there are no docstrings for that generatorWhat changes are included in this PR?
IpcDataGenerator
Are there any user-facing changes?
Better docs