You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/01/19 13:22:02 UTC
[GitHub] [arrow] alamb opened a new pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
alamb opened a new pull request #9263:
URL: https://github.com/apache/arrow/pull/9263
I found this while removing `test::format_batches` (PR to come shortly); The Record batch pretty printing code was printing numbers rather than dates.
Before this PR, when date/time columns were printed they were printed as numbers:
```
[
"+----------+",
"| f |",
"+----------+",
"| 11111111 |",
"| |",
"+----------+",
]
```
After this PR, they are printed (via chrono) as dates:
```
[
"+---------------------+",
"| f |",
"+---------------------+",
"| 1970-05-09 14:25:11 |",
"| |",
"+---------------------+",
]
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560314281
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
Wouldn't this be better as `NULL` and add a test for it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb closed pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9263:
URL: https://github.com/apache/arrow/pull/9263
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#issuecomment-762850821
https://issues.apache.org/jira/browse/ARROW-11318
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560316105
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_date($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING DATE"))
+ };
+
+ Ok(s)
+ }};
+}
+
+macro_rules! make_string_time {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_time($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING TIME"))
+ };
+
+ Ok(s)
+ }};
+}
+
+macro_rules! make_string_datetime {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_datetime($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING DATETIME"))
Review comment:
```suggestion
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560171845
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -104,30 +155,30 @@ 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::Timestamp(unit, _) if *unit == TimeUnit::Second => {
- make_string!(array::TimestampSecondArray, column, row)
+ make_string_datetime!(array::TimestampSecondArray, column, row)
}
DataType::Timestamp(unit, _) if *unit == TimeUnit::Millisecond => {
- make_string!(array::TimestampMillisecondArray, column, row)
+ make_string_datetime!(array::TimestampMillisecondArray, column, row)
}
DataType::Timestamp(unit, _) if *unit == TimeUnit::Microsecond => {
- make_string!(array::TimestampMicrosecondArray, column, row)
+ make_string_datetime!(array::TimestampMicrosecondArray, column, row)
}
DataType::Timestamp(unit, _) if *unit == TimeUnit::Nanosecond => {
- make_string!(array::TimestampNanosecondArray, column, row)
+ make_string_datetime!(array::TimestampNanosecondArray, column, row)
}
- DataType::Date32(_) => make_string!(array::Date32Array, column, row),
- DataType::Date64(_) => make_string!(array::Date64Array, column, row),
+ DataType::Date32(_) => make_string_date!(array::Date32Array, column, row),
+ DataType::Date64(_) => make_string_date!(array::Date64Array, column, row),
DataType::Time32(unit) if *unit == TimeUnit::Second => {
- make_string!(array::Time32SecondArray, column, row)
+ make_string_time!(array::Time32SecondArray, column, row)
}
DataType::Time32(unit) if *unit == TimeUnit::Millisecond => {
- make_string!(array::Time32MillisecondArray, column, row)
+ make_string_time!(array::Time32MillisecondArray, column, row)
}
- DataType::Time32(unit) if *unit == TimeUnit::Microsecond => {
- make_string!(array::Time64MicrosecondArray, column, row)
+ DataType::Time64(unit) if *unit == TimeUnit::Microsecond => {
Review comment:
There was actually a bug here -- there is no such type as using `Time32` for storing Microseconds -- it is actually `Time64` (as can be seen in the `Time64MicrosecondArray` type below it)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560339129
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
I think there are tradeoffs between using empty string or some sentinel (`"NULL"`) for display; I don't have a super strong preference though I do think the behavior should be consistent across types
This PR follows the same convention as the rest of this module and uses "" for NULL values: https://github.com/apache/arrow/pull/9263/files#diff-821be3a8a9239cdd12ab2b3c979e174f7b936aa6bfad4b9bc76489b9923cf747R38
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560315961
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_date($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING DATE"))
+ };
+
+ Ok(s)
+ }};
+}
+
+macro_rules! make_string_time {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_time($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING TIME"))
Review comment:
```suggestion
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560315711
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
+ } else {
+ array
+ .value_as_date($row)
+ .map(|d| d.to_string())
+ .unwrap_or_else(|| String::from("ERROR CONVERTING DATE"))
Review comment:
```suggestion
.unwrap_or_else(|| "ERROR CONVERTING DATE".to_string())
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560364731
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
Makes sense to me. I think the "problem" would be when displaying strings, as the empty string is displayed the same way as null, but I agree it is better to keep it consistent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560339965
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
The NULL behavior is included in the tests below (and the empty string is visible)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560314281
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
Wouldn't this be better as `"NULL"` and add a test for it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on a change in pull request #9263: ARROW-11318: [Rust] Support pretty printing timestamp, date, and timestamp types
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9263:
URL: https://github.com/apache/arrow/pull/9263#discussion_r560314281
##########
File path: rust/arrow/src/util/display.rs
##########
@@ -44,6 +44,57 @@ macro_rules! make_string {
}};
}
+macro_rules! make_string_date {
+ ($array_type:ty, $column: ident, $row: ident) => {{
+ let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+
+ let s = if array.is_null($row) {
+ "".to_string()
Review comment:
Wouldn't nulls be better displayed as `"NULL"` and add a test for it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org