alamb commented on code in PR #2939:
URL: https://github.com/apache/arrow-rs/pull/2939#discussion_r1007340557
##########
arrow/src/util/display.rs:
##########
@@ -190,14 +191,37 @@ macro_rules! make_string_datetime {
} else {
array
.value_as_datetime($row)
- .map(|d| d.to_string())
+ .map(|d| format!("{:?}", d))
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
};
Ok(s)
}};
}
+macro_rules! make_string_datetime_with_tz {
+ ($array_type:ty, $tz_string: ident, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ match $tz_string.parse::<Tz>() {
Review Comment:
as written this is going to parse the timezone string for every row --
perhaps we can parse it once per array 🤔
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -116,40 +119,40 @@ pub type Float64Array = PrimitiveArray<Float64Type>;
/// # Example: UTC timestamps post epoch
/// ```
/// # use arrow_array::TimestampSecondArray;
-/// use chrono::FixedOffset;
+/// use arrow_array::timezone::Tz;
/// // Corresponds to single element array with entry 1970-05-09T14:25:11+0:00
/// let arr = TimestampSecondArray::from(vec![11111111]);
/// // OR
/// let arr = TimestampSecondArray::from(vec![Some(11111111)]);
-/// let utc_offset = FixedOffset::east(0);
+/// let utc_tz: Tz = "+00:00".parse().unwrap();
Review Comment:
I think it would be nice to show an example of going from a `chrono`
timezone (like `FixedOffset`) to a Arrow::tz (though maybe that is already
available -- I didn't check)
##########
arrow/src/util/pretty.rs:
##########
@@ -370,13 +370,134 @@ mod tests {
};
}
+ /// Generate an array with type $ARRAYTYPE with a numeric value of
+ /// $VALUE, and compare $EXPECTED_RESULT to the output of
+ /// formatting that array with `pretty_format_batches`
+ macro_rules! check_datetime_with_timezone {
Review Comment:
As in "make them functions" 👍
##########
arrow/src/util/display.rs:
##########
@@ -190,14 +191,37 @@ macro_rules! make_string_datetime {
} else {
array
.value_as_datetime($row)
- .map(|d| d.to_string())
+ .map(|d| format!("{:?}", d))
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
};
Ok(s)
}};
}
+macro_rules! make_string_datetime_with_tz {
+ ($array_type:ty, $tz_string: ident, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ match $tz_string.parse::<Tz>() {
+ Ok(tz) => array
+ .value_as_datetime_with_tz($row, tz)
+ .map(|d| format!("{}", d.to_rfc3339()))
+ .unwrap_or_else(|| "ERROR CONVERTING DATE".to_string()),
+ Err(_) => array
+ .value_as_datetime($row)
+ .map(|d| format!("{:?} (Unknown Time Zone '{}')", d,
$tz_string))
Review Comment:
This doesn't seem right -- isn't this error for the case when
`$ts_string.parse()` fails? As written I think it will be returned when the
entry in array at $row is null (which should never happen).
I think we should remove the match statement
##########
arrow/src/util/display.rs:
##########
@@ -190,14 +191,37 @@ macro_rules! make_string_datetime {
} else {
array
.value_as_datetime($row)
- .map(|d| d.to_string())
+ .map(|d| format!("{:?}", d))
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
};
Ok(s)
}};
}
+macro_rules! make_string_datetime_with_tz {
Review Comment:
it might also be worth commenting that date/times are formatted as rfc3339?
##########
arrow/src/util/display.rs:
##########
@@ -334,17 +358,55 @@ pub fn array_value_to_string(column: &array::ArrayRef,
row: usize) -> Result<Str
DataType::Float32 => make_string!(array::Float32Array, column, row),
DataType::Float64 => make_string!(array::Float64Array, column, row),
DataType::Decimal128(..) => make_string_from_decimal(column, row),
- DataType::Timestamp(unit, _) if *unit == TimeUnit::Second => {
- make_string_datetime!(array::TimestampSecondArray, column, row)
+ DataType::Timestamp(unit, tz_string_opt) if *unit == TimeUnit::Second
=> {
+ match tz_string_opt {
+ Some(tz_string) => make_string_datetime_with_tz!(
Review Comment:
It would also be awesome to avoid having to make a string at all (like
return a `impl Display`) but that is definitely not something to do in this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
