You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2025/05/20 14:48:05 UTC
[PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
zhuqi-lucas opened a new pull request, #7534:
URL: https://github.com/apache/arrow-rs/pull/7534
…rray with i64::MIN / i64::MAX
# Which issue does this PR close?
[Closes #7533](https://github.com/apache/arrow-rs/issues/7533)
# Rationale for this change
Support panic handling and print invalid result.
Also added testing
```rust
#[test]
fn duration_pretty_and_iso_extremes() {
// 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();
assert_eq!(pretty.matches("<invalid>").count(), 2);
assert!(pretty.contains("0 days 1 hours 1 mins 1 secs"));
assert!(pretty.contains("<NULL>"));
// 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();
assert_eq!(iso.matches("<invalid>").count(), 2);
assert!(iso.contains("PT3661S"));
assert!(iso.contains("<NULL>"));
}
```
# What changes are included in this PR?
Support panic handling and print invalid result.
# Are there any user-facing changes?
Support panic handling and print invalid result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100297464
##########
arrow-cast/src/pretty.rs:
##########
@@ -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() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2099259412
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
Thank you @alamb for good suggestions! Addressed the option1 in latest PR, i think it's more clear.
##########
arrow-cast/Cargo.toml:
##########
@@ -37,6 +37,7 @@ all-features = true
[features]
prettyprint = ["comfy-table"]
+default = ["prettyprint"]
Review Comment:
Addressed in latest PR, thanks @alamb .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100164894
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
> Or, we can make the macro support both cases, if we want to remove option for those don't need:
This is a great idea. I have tried it in :https://github.com/zhuqi-lucas/arrow-rs/pull/3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#issuecomment-2898338845
I'll plan to merge this tomorrow unless there are any concerns with
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2099260107
##########
arrow-cast/src/display.rs:
##########
@@ -1234,4 +1249,32 @@ mod tests {
array_value_to_string(&map_array, 3).unwrap()
);
}
+
+ #[test]
+ fn duration_pretty_and_iso_extremes() {
+ // 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();
+ assert_eq!(pretty.matches("<invalid>").count(), 2);
+ assert!(pretty.contains("0 days 1 hours 1 mins 1 secs"));
+ assert!(pretty.contains("<NULL>"));
Review Comment:
Addressed in latest PR, thanks @alamb!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100001886
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
Thank you @zhuqi-lucas -- I have a suggestion here https://github.com/zhuqi-lucas/arrow-rs/pull/2
I think it would be nice to avoid having to use `try_duration_us_to_duration` and handle a `None` when that function can not return None. Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2098467513
##########
arrow-cast/Cargo.toml:
##########
@@ -37,6 +37,7 @@ all-features = true
[features]
prettyprint = ["comfy-table"]
+default = ["prettyprint"]
Review Comment:
I think we could just put the tests in the prettyprint module and avoid having to change any feature flags
##########
arrow-cast/src/display.rs:
##########
@@ -1234,4 +1249,32 @@ mod tests {
array_value_to_string(&map_array, 3).unwrap()
);
}
+
+ #[test]
+ fn duration_pretty_and_iso_extremes() {
+ // 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>");
Review Comment:
perhaps these tests can be put into the `pretty` module directly which would mean we don't have to adjust feature flags
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
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:
```rust
#[inline]
#[deprecated(since = "55.2.0", note = "Use `try_duration_s_to_duration` instead")]
pub fn duration_s_to_duration(v: i64) -> Duration {
Duration::try_seconds(v).unwrap()
}
#[inline]
pub fn try_duration_s_to_duration(v: i64) -> Option<Duration> {
Duration::try_seconds(v)
}
```
And similarly for `milliseconds`
## Option 2: Reexport `Duration`
Since these are thin wrappers over Chrono anyways, we could simply re-export Duration 🤔
```rust
// Reexport Duration for use by downstream libraries
pub use chrono::Duration;
#[inline]
#[deprecated(since = "55.2.0", note = "Use `Duration::try_seconds` instead")]
pub fn duration_s_to_duration(v: i64) -> Duration {
Duration::try_seconds(v).unwrap()
}
```
##########
arrow-cast/src/display.rs:
##########
@@ -1234,4 +1249,32 @@ mod tests {
array_value_to_string(&map_array, 3).unwrap()
);
}
+
+ #[test]
+ fn duration_pretty_and_iso_extremes() {
+ // 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();
+ assert_eq!(pretty.matches("<invalid>").count(), 2);
+ assert!(pretty.contains("0 days 1 hours 1 mins 1 secs"));
+ assert!(pretty.contains("<NULL>"));
Review Comment:
I think it would be easier to validate this test if you just compared the output entirely
Perhaps you can follow the model of the existing pretty print tests: https://github.com/apache/arrow-rs/blob/5b44d6702984fbac8a0faa93b85ec4c1120decd4/arrow-cast/src/pretty.rs#L459-L471
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100245177
##########
arrow-cast/src/pretty.rs:
##########
@@ -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() {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#issuecomment-2896495713
> Thank you @zhuqi-lucas for this contribution ❤️
Thank you @alamb for review and good suggestions! Addressed in latest PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100187377
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
Great idea -- proposed in https://github.com/zhuqi-lucas/arrow-rs/pull/3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Panic in pretty_format function when displaying DurationSecondsA… [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #7534:
URL: https://github.com/apache/arrow-rs/pull/7534#discussion_r2100046767
##########
arrow-array/src/temporal_conversions.rs:
##########
@@ -215,28 +215,28 @@ pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
(v.div_euclid(base), v.rem_euclid(base) as u32)
}
-/// converts a `i64` representing a `duration(s)` to [`Duration`]
+/// converts a `i64` representing a `duration(s)` to [`Option<Duration>`]
Review Comment:
Thank you @alamb , i tried this way, but it will make the following function failed with match fmt with Some():
```rust
macro_rules! duration_display {
($convert:ident, $t:ty, $scale:tt) => {
impl<'a> DisplayIndexState<'a> for &'a PrimitiveArray<$t> {
type State = DurationFormat;
fn prepare(&self, options: &FormatOptions<'a>) -> Result<Self::State, ArrowError> {
Ok(options.duration_format)
}
fn write(&self, fmt: &Self::State, idx: usize, f: &mut dyn Write) -> FormatResult {
let v = self.value(idx);
match fmt {
DurationFormat::ISO8601 => match $convert(v) {
Some(td) => write!(f, "{}", td)?,
None => write!(f, "<invalid>")?,
},
DurationFormat::Pretty => match $convert(v) {
Some(_) => duration_fmt!(f, v, $scale)?,
None => write!(f, "<invalid>")?,
},
}
Ok(())
}
}
};
}
```
The original one will always pass option:
```rust
duration_display!(try_duration_s_to_duration, DurationSecondType, 0);
duration_display!(try_duration_ms_to_duration, DurationMillisecondType, 3);
duration_display!(try_duration_us_to_duration, DurationMicrosecondType, 6);
duration_display!(try_duration_ns_to_duration, DurationNanosecondType, 9);
```
Or, we can make the macro support both cases, if we want to remove option for those don't need:
```rust
macro_rules! duration_display {
//–– converters that return Option<Duration> ––
($convert:ident, $t:ty, $scale:tt, option) => {
impl<'a> DisplayIndexState<'a> for &'a PrimitiveArray<$t> {
type State = DurationFormat;
fn prepare(&self, options: &FormatOptions<'a>) -> Result<Self::State, ArrowError> {
Ok(options.duration_format)
}
fn write(&self, fmt: &Self::State, idx: usize, f: &mut dyn Write) -> FormatResult {
let v = self.value(idx);
match fmt {
DurationFormat::ISO8601 => {
if let Some(td) = $convert(v) {
write!(f, "{}", td)?;
} else {
write!(f, "<invalid>")?;
}
}
DurationFormat::Pretty => {
if $convert(v).is_some() {
duration_fmt!(f, v, $scale)?;
} else {
write!(f, "<invalid>")?;
}
}
}
Ok(())
}
}
};
//–– converters that return plain Duration ––
($convert:ident, $t:ty, $scale:tt) => {
impl<'a> DisplayIndexState<'a> for &'a PrimitiveArray<$t> {
type State = DurationFormat;
fn prepare(&self, options: &FormatOptions<'a>) -> Result<Self::State, ArrowError> {
Ok(options.duration_format)
}
fn write(&self, fmt: &Self::State, idx: usize, f: &mut dyn Write) -> FormatResult {
let v = self.value(idx);
match fmt {
DurationFormat::ISO8601 => {
write!(f, "{}", $convert(v))?;
}
DurationFormat::Pretty => {
duration_fmt!(f, v, $scale)?;
}
}
Ok(())
}
}
};
}
```
And call it by:
```rust
// those four lines at the top of your file become:
// these two return Option<Duration>:
duration_display!(try_duration_s_to_duration, DurationSecondType, 0, option);
duration_display!(try_duration_ms_to_duration, DurationMillisecondType, 3, option);
// these two return Duration:
duration_display!(duration_us_to_duration, DurationMicrosecondType, 6);
duration_display!(duration_ns_to_duration, DurationNanosecondType, 9);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org