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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
