-
Notifications
You must be signed in to change notification settings - Fork 937
fix: Panic in pretty_format function when displaying DurationSecondsA… #7534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0f67f29
868057b
70efb5e
7d90c55
45ba5dc
e62e594
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,7 +221,7 @@ mod tests { | |
use arrow_buffer::{IntervalDayTime, IntervalMonthDayNano, ScalarBuffer}; | ||
use arrow_schema::*; | ||
|
||
use crate::display::array_value_to_string; | ||
use crate::display::{array_value_to_string, DurationFormat}; | ||
|
||
use super::*; | ||
|
||
|
@@ -1186,4 +1186,56 @@ mod tests { | |
let actual: Vec<&str> = batch.lines().collect(); | ||
assert_eq!(expected_table, actual, "Actual result:\n{batch}"); | ||
} | ||
|
||
#[test] | ||
fn duration_pretty_and_iso_extremes() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing that would be good would be to extend these tests to cover the other duration TimeUnits (like milliseconds, microseconds and nanoseconds) However I don't think it is required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @alamb , I think it's a good point, because it seems no other testing cover it, i will create a follow-up PR to improve the test. |
||
// Build [MIN, MAX, 3661, NULL] | ||
let arr = DurationSecondArray::from(vec![Some(i64::MIN), Some(i64::MAX), Some(3661), None]); | ||
let array: ArrayRef = Arc::new(arr); | ||
|
||
// Pretty formatting | ||
let opts = FormatOptions::default().with_null("null"); | ||
let opts = opts.with_duration_format(DurationFormat::Pretty); | ||
let pretty = pretty_format_columns_with_options("pretty", &[array.clone()], &opts) | ||
.unwrap() | ||
.to_string(); | ||
|
||
// Expected output | ||
let expected_pretty = vec![ | ||
"+------------------------------+", | ||
"| pretty |", | ||
"+------------------------------+", | ||
"| <invalid> |", | ||
"| <invalid> |", | ||
"| 0 days 1 hours 1 mins 1 secs |", | ||
"| null |", | ||
"+------------------------------+", | ||
]; | ||
|
||
let actual: Vec<&str> = pretty.lines().collect(); | ||
assert_eq!(expected_pretty, actual, "Actual result:\n{pretty}"); | ||
|
||
// ISO8601 formatting | ||
let opts_iso = FormatOptions::default() | ||
.with_null("null") | ||
.with_duration_format(DurationFormat::ISO8601); | ||
let iso = pretty_format_columns_with_options("iso", &[array], &opts_iso) | ||
.unwrap() | ||
.to_string(); | ||
|
||
// Expected output | ||
let expected_iso = vec![ | ||
"+-----------+", | ||
"| iso |", | ||
"+-----------+", | ||
"| <invalid> |", | ||
"| <invalid> |", | ||
"| PT3661S |", | ||
"| null |", | ||
"+-----------+", | ||
]; | ||
|
||
let actual: Vec<&str> = iso.lines().collect(); | ||
assert_eq!(expected_iso, actual, "Actual result:\n{iso}"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these methods are part of the public API : https://docs.rs/arrow/latest/arrow/array/temporal_conversions/fn.duration_s_to_duration.html
Thus we can't change them until the next major release (July 2025)
Option 1:
We could leave the original signatures and deprecate the two that are actually fallible per https://github.com/apache/arrow-rs?tab=readme-ov-file#deprecation-guidelines
And we can then add new functions like
try_
for the two fallible conversions:And similarly for
milliseconds
Option 2: Reexport
Duration
Since these are thin wrappers over Chrono anyways, we could simply re-export Duration 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @alamb for good suggestions! Addressed the option1 in latest PR, i think it's more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @zhuqi-lucas -- I have a suggestion here zhuqi-lucas#2
I think it would be nice to avoid having to use
try_duration_us_to_duration
and handle aNone
when that function can not return None. Let me know what you thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @alamb , i tried this way, but it will make the following function failed with match fmt with Some():
The original one will always pass option:
Or, we can make the macro support both cases, if we want to remove option for those don't need:
And call it by:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea. I have tried it in :zhuqi-lucas#3
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.