ARROW-3878: [Rust] Improve primitive types #3031
Conversation
|
@kszucs @paddyhoran @andygrove could you review this? Thanks. |
| @@ -15,6 +15,8 @@ | |||
| // specific language governing permissions and limitations | |||
| // under the License. | |||
|
|
|||
| #![feature(specialization)] | |||
There was a problem hiding this comment.
Can you explain how we are now using the specialization feature?
There was a problem hiding this comment.
This is used in BufferBuilder where we defined a trait BufferBuilderTrait (I'm not too happy with this name..) and implemented this separately for BufferBuilder<T: ArrowNumericType> and BufferBuilder<BooleanType>. Without this, we'd have to implement PrimitiveArrayBuilder separately for ArrowNumericType and BooleanType, with the same code.
|
@sunchao I think this needs a re-base after merging ARROW-3855 @andygrove does the merge script automatically rebase before merging? |
|
The merge script rebases, yes |
rust/src/array.rs
Outdated
| @@ -24,7 +24,7 @@ use std::sync::Arc; | |||
|
|
|||
| use array_data::*; | |||
| use buffer::*; | |||
There was a problem hiding this comment.
I there is a need to use the use <module>::* pattern for builder and datatypes (and array) because they define a lot of types, but maybe we should avoid this for array_data and buffer?
There was a problem hiding this comment.
Overall, I am loving this update. The macros were making it very hard to build higher level types. Just a couple of nits.
Thanks @sunchao
rust/src/array.rs
Outdated
|
|
||
| fn data_ref(&self) -> &ArrayDataRef { | ||
| &self.data | ||
| } | ||
| } | ||
|
|
||
| /// Macro to define primitive arrays for different data types and native types. | ||
| /// Boolean arrays are bit-packed and so are not defined by this macro |
There was a problem hiding this comment.
The comments need to be updated here as the macro is gone.
| pub type Float32BufferBuilder = BufferBuilder<Float32Type>; | ||
| pub type Float64BufferBuilder = BufferBuilder<Float64Type>; | ||
|
|
||
| pub trait BufferBuilderTrait<T: ArrowPrimitiveType> { |
There was a problem hiding this comment.
Docs need to be updated for BufferBuilderTrait in general. Also, is there a way to avoid anonymous function parameters as these are deprecated, see RFC-1685
rust/src/datatypes.rs
Outdated
| type Native: ArrowNativeType; | ||
|
|
||
| /// Returns the id of this primitive type. | ||
| fn get_type_id() -> DataType; |
There was a problem hiding this comment.
nit: I think type_id is the term used in the C++ impl, but we have used data_type. Should we stick with data_type everywhere or convert to use type_id everywhere?
No description provided.