Configurable Duration Display#4581
Conversation
| true => { | ||
| write!( | ||
| $f, | ||
| concat!("{} days {} hours {} mins -{}.{:0", $scale, "} secs"), |
There was a problem hiding this comment.
I opted to not include years and months as durations can never store these quantities and so including them seemed redundant
arrow-cast/src/display.rs
Outdated
| /// * Human Readable - "198 days 16 hours 34 mins 15.407810000 secs" | ||
| /// | ||
| /// Defaults to true | ||
| pub const fn with_iso8601_duration_format(self, iso8601: bool) -> Self { |
There was a problem hiding this comment.
I could see us also adding an option to print intervals as ISO8601
There was a problem hiding this comment.
Rather than a bool here, an enum that would allow more easily adding different formats in the future without changing the API.
Perhaps something like
enum DurationFormat {
/// Human Readable - "198 days 16 hours 34 mins 15.407810000 secs"
Human,
/// ISO 8601 - "P198DT72932.972880S"
ISO8601,
}
...
pub const with_duration_format(mut self, format: DurationFormat) {
...
}
| assert_eq!(formatted.value(1).to_string(), "[[4], [null], [6]]"); | ||
| } | ||
|
|
||
| const CAST_OPTIONS: CastOptions<'static> = CastOptions { |
There was a problem hiding this comment.
This will let us define constant CastOptions and FormatOptions within DataFusion to override the way it formats durations
arrow-cast/src/display.rs
Outdated
| assert_eq!(iso[1], "-PT0.000000001S"); | ||
| assert_eq!(non_iso[1], "0 days 0 hours 0 mins -0.000000001 secs"); | ||
| assert_eq!(iso[2], "PT0.000001S"); | ||
| assert_eq!(non_iso[2], "0 days 0 hours 0 mins 0.000001000 secs"); |
There was a problem hiding this comment.
I was a little torn on whether to print trailing 0s, we currently do for intervals and don't for timestamps. As we are trying to emulate the former, I opted to follow that convention
| mod tests { | ||
| use super::*; | ||
|
|
||
| const TEST_CONST_OPTIONS: FormatOptions<'static> = FormatOptions::new() |
There was a problem hiding this comment.
I think a comment explaining the rationale of the test (so it isn't inadvertently remove) would be helpful
| const TEST_CONST_OPTIONS: FormatOptions<'static> = FormatOptions::new() | |
| // Test to verify options can be constant. See | |
| // https://github.com/apache/arrow-rs/issues/4580 | |
| const TEST_CONST_OPTIONS: FormatOptions<'static> = FormatOptions::new() |
arrow-cast/src/display.rs
Outdated
| /// * Human Readable - "198 days 16 hours 34 mins 15.407810000 secs" | ||
| /// | ||
| /// Defaults to true | ||
| pub const fn with_iso8601_duration_format(self, iso8601: bool) -> Self { |
There was a problem hiding this comment.
Rather than a bool here, an enum that would allow more easily adding different formats in the future without changing the API.
Perhaps something like
enum DurationFormat {
/// Human Readable - "198 days 16 hours 34 mins 15.407810000 secs"
Human,
/// ISO 8601 - "P198DT72932.972880S"
ISO8601,
}
...
pub const with_duration_format(mut self, format: DurationFormat) {
...
}2 participants
Which issue does this PR close?
Closes #4580
Closes #4554
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?