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) {
...
}
| @@ -8959,4 +8959,14 @@ mod tests { | |||
| assert_eq!(formatted.value(0).to_string(), "[[1], [2], [3]]"); | |||
| 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
| @@ -866,6 +951,15 @@ pub fn lexical_to_string<N: lexical_core::ToLexical>(n: N) -> String { | |||
| 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) {
...
}
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?