ARROW-3878: [Rust] Improve primitive types#3031
Conversation
|
@kszucs @paddyhoran @andygrove could you review this? Thanks. |
andygrove
left a comment
There was a problem hiding this comment.
LGTM - I like having the specific numeric type trait.
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 |
| @@ -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?
paddyhoran
left a comment
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
| } | ||
|
|
||
| /// 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
| 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?
paddyhoran
left a comment
There was a problem hiding this comment.
LGTM, thanks @sunchao this is a great improvement!
No description provided.