ARROW-6944: [Rust] Add String, FixedSizeBinary types#5722
ARROW-6944: [Rust] Add String, FixedSizeBinary types#5722nevi-me wants to merge 7 commits intoapache:masterfrom
Conversation
|
I need to add/clean up documentation, and add more tests, but otherwise the types and their arrays are working. |
|
@andygrove @paddyhoran @liurenjie1024 PTAL. This completes the basic types that we need for most of our integration testing. Other types like Decimal, Timestamp with timezone would be nice, but I don't need them to complete the work on #4167 |
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Left some comments.
There was a problem hiding this comment.
This may be unnecessary because Buffer already has alignment check
There was a problem hiding this comment.
I removed it and got test failures, I've left it in for now
paddyhoran
left a comment
paddyhoran
left a comment
There was a problem hiding this comment.
Couple of nits, thanks @nevi-me
There was a problem hiding this comment.
Shouldn't we be checking that i + offset < len instead of just i?
Also, returning a Result is probably unnecessary but maybe use expect instead of unwrap.
There was a problem hiding this comment.
Same 2 points as above.
|
@paddyhoran @liurenjie1024 I've addressed review comments |
|
LGTM. Thanks @nevi-me |
4 participants
This PR creates a
StringArraywhich behaves exactly like the currentBinaryArray, then restricts theBinaryArrayto dealing with binary data only. It also adds aFixedSizeBinaryArraywhich is backed by aFixedSizeListArray.