alamb commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1081145188
##########
arrow-cast/src/display.rs:
##########
@@ -309,9 +379,10 @@ fn append_map_field_string(
///
/// Note this function is quite inefficient and is unlikely to be
/// suitable for converting large arrays or record batches.
-pub fn array_value_to_string(
+fn array_value_to_string_internal(
column: &ArrayRef,
row: usize,
+ datetime_format_opt: &Option<String>,
Review Comment:
FYI I think the typical rust way to do this (and not require an owned
`String`) is:
```suggestion
datetime_format_opt: Option<&str>,
```
And then at callsites with `val: Option<String>` instead of:
```rust
array_value_to_string_internal(foo, bar, &val)
```
You do :
```rust
array_value_to_string_internal(foo, bar, val.as_ref)
```
##########
arrow-csv/src/writer.rs:
##########
@@ -430,33 +364,45 @@ impl WriterBuilder {
self
}
+ /// Whether to use RFC3339 format for date/time/timestamps
Review Comment:
```suggestion
/// Use RFC3339 format for date/time/timestamps by clearing all
/// date/time specific formats.
```
##########
arrow-csv/src/writer.rs:
##########
@@ -601,75 +547,9 @@ sed do eiusmod
tempor,-556132.25,1,,2019-04-18T02:45:55.555000000,23:46:03,foo
file.read_to_end(&mut buffer).unwrap();
assert_eq!(
- "Lorem ipsum dolor sit amet|123.564532|3|true|12:20:34
AM\nconsectetur adipiscing elit|NULL|2|false|06:51:20 AM\nsed do eiusmod
tempor|-556132.25|1|NULL|11:46:03 PM\n"
+ "Lorem ipsum dolor sit
amet|123.564532|3|true|00:20:34\nconsectetur adipiscing
elit|NULL|2|false|06:51:20\nsed do eiusmod tempor|-556132.25|1|NULL|23:46:03\n"
.to_string(),
String::from_utf8(buffer).unwrap()
);
}
-
- #[test]
- fn test_conversion_consistency() {
Review Comment:
Why was this test removed? It seems useful, doesn't it?
##########
arrow-cast/src/display.rs:
##########
@@ -510,6 +591,21 @@ pub fn array_value_to_string(
}
}
+pub fn array_value_to_string(
+ column: &ArrayRef,
+ row: usize,
+) -> Result<String, ArrowError> {
+ array_value_to_string_internal(column, row, &None)
+}
+
+pub fn datetime_array_value_to_string(
+ column: &ArrayRef,
+ row: usize,
+ format: &Option<String>,
Review Comment:
```suggestion
format: Option<&str>,
```
##########
arrow-cast/src/display.rs:
##########
@@ -510,6 +591,21 @@ pub fn array_value_to_string(
}
}
+pub fn array_value_to_string(
+ column: &ArrayRef,
+ row: usize,
+) -> Result<String, ArrowError> {
+ array_value_to_string_internal(column, row, &None)
+}
+
+pub fn datetime_array_value_to_string(
Review Comment:
I would love to revamp this display code to be easier to use / more
performant but I think that is outside the scope of this PR. I'll try and file
a ticket explaining what I am thinking
##########
arrow-cast/src/display.rs:
##########
@@ -510,6 +591,21 @@ pub fn array_value_to_string(
}
}
+pub fn array_value_to_string(
+ column: &ArrayRef,
+ row: usize,
+) -> Result<String, ArrowError> {
+ array_value_to_string_internal(column, row, &None)
+}
+
+pub fn datetime_array_value_to_string(
Review Comment:
I am somewhat worried about adding a new public API like this, but I see it
is needed to call in arrow-csv
##########
arrow/tests/csv.rs:
##########
@@ -57,8 +57,8 @@ fn test_export_csv_timestamps() {
drop(writer);
let left = "c1,c2
-2019-04-18T20:54:47.378000000+10:00,2019-04-18T10:54:47.378000000
-2021-10-30T17:59:07.000000000+11:00,2021-10-30T06:59:07.000000000\n";
+2019-04-18T20:54:47.378+10:00,2019-04-18T10:54:47.378000000
Review Comment:
this sure looks better 😍
##########
arrow-csv/src/writer.rs:
##########
@@ -430,33 +364,45 @@ impl WriterBuilder {
self
}
+ /// Whether to use RFC3339 format for date/time/timestamps
+ pub fn with_rfc3339(mut self, use_rfc3339: bool) -> Self {
+ self.use_rfc3339 = use_rfc3339;
Review Comment:
Rather than an extra flag, what would you think about having this function
simply set
```rust
self.date_format = None;
self.datetime_format = None;
....
```
?
As a separate flag it might be confusing to understand what happens if
`use_rfc3339` is set and then a user also set `self.date_format` or something
--
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]
