ARROW-4377: [Rust] Implement std::fmt::Debug for PrimitiveArrays#3663
ARROW-4377: [Rust] Implement std::fmt::Debug for PrimitiveArrays#3663ntrinquier wants to merge 4 commits intoapache:masterfrom ntrinquier:arrow-4377
Conversation
|
Thanks @ntrinquier, can you change the title to "ARROW-4377: [Rust] Implement std::fmt::Debug for PrimitiveArrays" please. I'll take a look when I get a chance. |
|
Also, CI is failing due to formatting issues. We use nightly compiler but stable rust fmt so you can run |
ntrinquier
changed the title
Codecov Report
@@ Coverage Diff @@
## master #3663 +/- ##
==========================================
- Coverage 87.84% 87.78% -0.06%
==========================================
Files 689 689
Lines 84014 84014
Branches 1081 1081
==========================================
- Hits 73798 73752 -46
- Misses 10103 10151 +48
+ Partials 113 111 -2
Continue to review full report at Codecov.
|
nevi-me
left a comment
nevi-me
left a comment
There was a problem hiding this comment.
Thanks for taking this on Nicolas, please see my inline comment.
| } | ||
|
|
||
| #[test] | ||
| fn test_boolean_fmt_debug() { |
There was a problem hiding this comment.
May you please add tests for null values, I don't think you handle them because getting a null value returns invalid data.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Looking good so far. I'd like to see those unwraps removed in favor of returning an error result though (I'm guilty of using unwrap too, but I think it won't be hard to make the change here).
rust/arrow/src/array.rs
Outdated
| T::get_data_type(), | ||
| self.len() | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
It would be better to return fmt::Result(Error) rather than call unwrap here
paddyhoran
left a comment
paddyhoran
left a comment
There was a problem hiding this comment.
@ntrinquier what do you think about displaying the array values vertically?
This would be more consistent with C++/Python and make it easier to debug larger arrays.
Also, if we want to debug nested arrays in the future we can use the vertical indentation to help display the nesting levels.
|
@paddyhoran It leaves more unused space but I guess it is better to be consistent (plus as you said, with nesting it will actually be much better if displayed vertically). |
paddyhoran
left a comment
paddyhoran
left a comment
There was a problem hiding this comment.
Yep, LGTM, thanks @ntrinquier
|
CI failure is unrelated, merging. Thanks @ntrinquier! |
6 participants
No description provided.