[GitHub] [arrow-rs] waitingkuo commented on a diff in pull request #2939: Format Timestamps as RFC3339

[GitBox <gi...@apache.org>]

waitingkuo commented on code in PR #2939:
URL: https://github.com/apache/arrow-rs/pull/2939#discussion_r1007742288


##########
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:
   the logic behind is:
   
   while parsing TZ failed, parse datetime only
   1. if it works, show the datetime part with a unknown time zone warning
   ```bash
   +--------------------------------------------------------+
   | Timestamp(Second, Some("Asia/Taipei2"))                |
   +--------------------------------------------------------+
   | 1970-01-01T00:00:00 (Unknown Time Zone 'Asia/Taipei2') |
   +--------------------------------------------------------+
   ```
   2. if it doesn't work, display something (similar as `make_string_datetime`) like this
   ```bash
   +--------------------------------------------------------+
   | Timestamp(Second, Some("Asia/Taipei2"))                |
   +--------------------------------------------------------+
   | ERROR CONVERTING DATE                                  |
   +--------------------------------------------------------+
   ```
   
   @alamb @tustvold 
   I wonder if it's preferred. we could also show followings directly while failed to parse timezone
   ```bash
   +--------------------------------------------------------+
   | Timestamp(Second, Some("Asia/Taipei2"))                |
   +--------------------------------------------------------+
   | Unknown Time Zone 'Asia/Taipei2'                       |
   +--------------------------------------------------------+
   ```
   
   i agree the logic here is confusing, i should've added some comments here



-- 
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