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
ARROW-10336: [Rust] Added FromIter and ToIter for string arrays #8486
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.
I think type arguments should be always screaming snake case in whole codebase. Apart from that it looks ok.
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.
LGTM
@vertexclique , do you refer to the |
} | ||
} | ||
|
||
impl<'a, Ptr, OffsetSize: StringOffsetSizeTrait> FromIterator<Ptr> |
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 meant type params like:
impl<'a, Ptr, OffsetSize: StringOffsetSizeTrait> FromIterator<Ptr> | |
impl<'a, P, OS: StringOffsetSizeTrait> FromIterator<P> |
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 see. I have no strong feelings about it, I just think that explicit is better than implicit, and P
or OS
forces the reader to go to the where
clause to understand what they mean. OTOH, generics are generics, and also rust seems to prefer one-letter generic types.
I will leave it like this just because the rest of the code base around offsets is like this.
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.
Happy with it as is, if we need to change, we can do that for the whole module/codebase in future
What the title says: adds string array from and to iterators. This PR also simplifies some of the functions around strings. Closes apache#8486 from jorgecarleitao/from_opt_vec_str Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>
What the title says: adds string array from and to iterators.
This PR also simplifies some of the functions around strings.