-
Notifications
You must be signed in to change notification settings - Fork 906
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
Doc improvements #3155
Doc improvements #3155
Conversation
arrow-array/src/arithmetic.rs
Outdated
@@ -45,61 +45,83 @@ pub trait ArrowNativeTypeOp: ArrowNativeType { | |||
/// The multiplicative identity | |||
const ONE: Self; | |||
|
|||
/// A checked add operation |
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.
/// A checked add operation | |
/// Checked addition operation |
A
seems redundant to me.
arrow-array/src/arithmetic.rs
Outdated
fn add_checked(self, rhs: Self) -> Result<Self, ArrowError>; | ||
|
||
/// A wrapping add operation |
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.
/// A wrapping add operation | |
/// Wrapping addition operation |
arrow-array/src/arithmetic.rs
Outdated
fn add_wrapping(self, rhs: Self) -> Self; | ||
|
||
/// A checked subtract operation |
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.
/// A checked subtract operation | |
/// Checked subtraction operation |
arrow-array/src/arithmetic.rs
Outdated
fn sub_checked(self, rhs: Self) -> Result<Self, ArrowError>; | ||
|
||
/// A wrapping subtract operation |
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.
/// A wrapping subtract operation | |
/// Wrapping subtraction operation |
arrow-array/src/arithmetic.rs
Outdated
fn div_wrapping(self, rhs: Self) -> Self; | ||
|
||
/// A checked modulo operation |
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.
/// A checked modulo operation | |
/// Checked remainder operation |
arrow-array/src/arithmetic.rs
Outdated
fn mod_checked(self, rhs: Self) -> Result<Self, ArrowError>; | ||
|
||
/// A wrapping modulo operation |
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.
/// A wrapping modulo operation | |
/// Wrapping remainder operation |
arrow-array/src/array/list_array.rs
Outdated
@@ -29,7 +29,9 @@ use std::any::Any; | |||
|
|||
/// trait declaring an offset size, relevant for i32 vs i64 array types. | |||
pub trait OffsetSizeTrait: ArrowNativeType + std::ops::AddAssign + Integer { | |||
/// True for 64 bit size and false for 32 bit size |
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.
/// True for 64 bit size and false for 32 bit size | |
/// True for 64 bit offset size and false for 32 bit offset size |
/// An array where each element is a 128-bits decimal with precision in [1, 64] and | ||
/// scale in [0, 64]. |
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.
/// An array where each element is a 128-bits decimal with precision in [1, 64] and | |
/// scale in [0, 64]. | |
/// An array where each element is a 128-bits decimal with precision in [1, 76] and | |
/// scale in [0, 76]. |
#[inline] | ||
/// Returns the length of the buffer |
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 think we used to put doc on top of #[inline]
.
#[inline] | |
/// Returns the length of the buffer | |
/// Returns the length of the buffer | |
#[inline] |
#[inline] | ||
/// Appends n `additional` bits of value `v` into the buffer |
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.
#[inline] | |
/// Appends n `additional` bits of value `v` into the buffer | |
/// Appends `additional` bits of value `v` into the buffer | |
#[inline] |
#[inline] | ||
/// Creates a [`Buffer`] |
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.
#[inline] | |
/// Creates a [`Buffer`] | |
/// Creates a [`Buffer`] from this builder and resets this builder. | |
#[inline] |
pub type Float64BufferBuilder = BufferBuilder<f64>; | ||
|
||
/// A timestamp second array builder. |
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.
/// A timestamp second array builder. | |
/// Buffer builder for timestamp type of second unit. |
pub type TimestampNanosecondBufferBuilder = | ||
BufferBuilder<<TimestampNanosecondType as ArrowPrimitiveType>::Native>; | ||
|
||
/// A 32-bit date array builder. |
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.
/// A 32-bit date array builder. | |
/// Buffer builder for 32-bit date type |
/// builder.append(false).unwrap(); | ||
/// builder.append(true).unwrap(); | ||
/// | ||
/// let arr = builder.finish(); |
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 think it would be better to add few assert
s which confirms the values of built array. Like the docs of other builders do.
#[derive(Debug, Clone)] | ||
pub struct MapFieldNames { | ||
/// [`Field`] name for map |
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.
/// [`Field`] name for map | |
/// [`Field`] name for map entries |
pub entry: String, | ||
/// [`Field`] name for key |
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.
/// [`Field`] name for key | |
/// [`Field`] name for map key |
pub key: String, | ||
/// [`Field`] name for value |
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.
/// [`Field`] name for value | |
/// [`Field`] name for map value |
@@ -78,10 +111,12 @@ impl<K: ArrayBuilder, V: ArrayBuilder> MapBuilder<K, V> { | |||
} | |||
} | |||
|
|||
/// Returns the keys of the map |
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.
/// Returns the keys of the map | |
/// Returns the key array builder of the map |
pub fn keys(&mut self) -> &mut K { | ||
&mut self.key_builder | ||
} | ||
|
||
/// Returns the values of the map |
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.
/// Returns the values of the map | |
/// Returns the value array builder of the map |
Thanks for the comments. I will do the changes and request a review again after I resolve them. |
pub type DurationNanosecondArray = PrimitiveArray<DurationNanosecondType>; | ||
|
||
/// An array where each element is a 128-bits decimal with precision in [1, 38] and | ||
/// scale in [0, 38]. |
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.
/// scale in [0, 38]. | |
/// scale in [-38, 38]. |
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.
My mad, missed seeing the negative scale support in the recent commit.
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.
Done
pub type Decimal128Array = PrimitiveArray<Decimal128Type>; | ||
/// An array where each element is a 256-bits decimal with precision in [1, 76] and | ||
/// scale in [0, 76]. |
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.
/// scale in [0, 76]. | |
/// scale in [-76, 76]. |
Benchmark runs are scheduled for baseline = 007fb4c and contender = 2460c7b. 2460c7b 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?
Partially implements #37 .
Rationale for this change
Improves documentation for arrow-array and json
What changes are included in this PR?
Adds a whole bunch of doc comments
Are there any user-facing changes?
No