Uh oh!
There was an error while loading. Please reload this page.
ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types - #9263
ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types#9263alamb wants to merge 2 commits into
Conversation
| } | ||
| DataType::Time32(unit) if *unit == TimeUnit::Microsecond => { | ||
| make_string!(array::Time64MicrosecondArray, column, row) | ||
| DataType::Time64(unit) if *unit == TimeUnit::Microsecond => { |
There was a problem hiding this comment.
There was actually a bug here -- there is no such type as using Time32 for storing Microseconds -- it is actually Time64 (as can be seen in the Time64MicrosecondArray type below it)
| let array = $column.as_any().downcast_ref::<$array_type>().unwrap(); | ||
| let s = if array.is_null($row) { | ||
| "".to_string() |
There was a problem hiding this comment.
Wouldn't nulls be better displayed as "NULL" and add a test for it?
There was a problem hiding this comment.
I think there are tradeoffs between using empty string or some sentinel ("NULL") for display; I don't have a super strong preference though I do think the behavior should be consistent across types
This PR follows the same convention as the rest of this module and uses "" for NULL values: https://github.com/apache/arrow/pull/9263/files#diff-821be3a8a9239cdd12ab2b3c979e174f7b936aa6bfad4b9bc76489b9923cf747R38
There was a problem hiding this comment.
The NULL behavior is included in the tests below (and the empty string is visible)
There was a problem hiding this comment.
Makes sense to me. I think the "problem" would be when displaying strings, as the empty string is displayed the same way as null, but I agree it is better to keep it consistent.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Daniël Heres <danielheres@gmail.com>
…stamp types I found this while removing `test::format_batches` (PR to come shortly); The Record batch pretty printing code was printing numbers rather than dates. Before this PR, when date/time columns were printed they were printed as numbers: ``` [ "+----------+", "| f |", "+----------+", "| 11111111 |", "| |", "+----------+", ] ``` After this PR, they are printed (via chrono) as dates: ``` [ "+---------------------+", "| f |", "+---------------------+", "| 1970-05-09 14:25:11 |", "| |", "+---------------------+", ] ``` Closes#9263 from alamb/alamb/pretty_print_datetimes Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
I found this while removing
test::format_batches(PR to come shortly); The Record batch pretty printing code was printing numbers rather than dates.Before this PR, when date/time columns were printed they were printed as numbers:
After this PR, they are printed (via chrono) as dates: