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 2023/01/12 01:03:40 UTC
[GitHub] [arrow-rs] JayjeetAtGithub opened a new pull request, #3514: Use array_value_to_string in arrow-csv
JayjeetAtGithub opened a new pull request, #3514:
URL: https://github.com/apache/arrow-rs/pull/3514
# Which issue does this PR close?
Closes #3483.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1381026756
i agree that keeping some way to specify the formatting is likely necessary and it would be great to add the ability to do so to `array_value_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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] JayjeetAtGithub commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1083321202
##########
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:
I agree. I am also thinking if the user first uses `with_rfc3339` and then `with_date_format`, then the later will override the former, and it might also be confusing?! We would probably need to explain this behavior properly in the API docs.
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1413735127
I'm going to merge this in so #3647 doesn't generate huge numbers of conflicts with 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
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())
```
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1396204210
Thanks @JayjeetAtGithub -- I plan to take a look tomorrow
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1408722485
I was inspired by this PR to write up a potential new API for formatting arrays as strings as the current one has bothered me for a long time: https://github.com/apache/arrow-rs/issues/3638
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1399385766
Hi @alamb , Looks like there is a conflict. Any ideas to handle this would be great. Thanks.
--
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
[GitHub] [arrow-rs] ursabot commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1413738673
Benchmark runs are scheduled for baseline = 2b9bbce44abbd93048c674f49c4eb0db72a0a1c8 and contender = 75a56beb827e86791325906886eaaa4c747a9ea2. 75a56beb827e86791325906886eaaa4c747a9ea2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/13eaed16b06a44bcb03e28bec75d9d45...6419d9fc391b4f44aff55a5b582864f4/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/81115d142ae54622b83610a9212d1f9c...2dce41b7580e47e192a8461de3bc9362/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/4887a3a8ddcd4b398f5d81664c1d3da1...5b395a0347d24999b86fe3691d669586/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ec2a60e58b8a4842934a3956befd8b26...4e2e1d1276b74747bbd5496a548a680c/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1090659093
##########
arrow-cast/src/display.rs:
##########
@@ -524,6 +721,22 @@ pub fn array_value_to_string(
}
}
+pub fn temporal_array_value_to_string(
Review Comment:
Can we please add some doc comments about what this does (specifically what `DataTypes` it accepts) and what the format is used for?
I realize you didn't add this function so we can do so as a follow on PR I think
##########
arrow-cast/src/display.rs:
##########
@@ -28,6 +28,13 @@ use arrow_array::*;
use arrow_buffer::ArrowNativeType;
use arrow_schema::*;
use chrono::prelude::SecondsFormat;
+use chrono::{DateTime, Utc};
+
+fn invalid_cast_error(dt: &str, col_idx: usize, row_idx: usize) -> ArrowError {
+ ArrowError::CastError(format!(
+ "Cannot cast to {dt} at col index: {col_idx} row index: {row_idx}"
Review Comment:
I like the addition of column index to the error message 👍
##########
arrow-csv/src/writer.rs:
##########
@@ -177,88 +158,74 @@ impl<W: Write> Writer<W> {
DataType::UInt16 => write_primitive_value::<UInt16Type>(col, row_index),
DataType::UInt32 => write_primitive_value::<UInt32Type>(col, row_index),
DataType::UInt64 => write_primitive_value::<UInt64Type>(col, row_index),
- DataType::Boolean => {
- let c = col.as_any().downcast_ref::<BooleanArray>().unwrap();
- c.value(row_index).to_string()
- }
- DataType::Utf8 => {
- let c = col.as_any().downcast_ref::<StringArray>().unwrap();
- c.value(row_index).to_owned()
- }
- DataType::LargeUtf8 => {
- let c = col.as_any().downcast_ref::<LargeStringArray>().unwrap();
- c.value(row_index).to_owned()
- }
- DataType::Date32 => {
- write_temporal_value!(
- col,
- Date32Array,
- &self.date_format,
- col_index,
- row_index,
- value_as_date,
- "Date32"
- )
- }
- DataType::Date64 => {
- write_temporal_value!(
+ DataType::Boolean => array_value_to_string(col, row_index)?.to_string(),
Review Comment:
👍 that certainly looks better
##########
arrow-cast/src/display.rs:
##########
@@ -133,57 +140,176 @@ macro_rules! make_string_interval_month_day_nano {
}
macro_rules! make_string_date {
- ($array_type:ty, $column: ident, $row: ident) => {{
- let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+ ($array_type:ty, $dt:expr, $column: ident, $col_idx:ident, $row_idx: ident) => {{
+ Ok($column
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .value_as_date($row_idx)
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .to_string())
+ }};
+}
- Ok(array
- .value_as_date($row)
- .map(|d| d.to_string())
- .unwrap_or_else(|| "ERROR CONVERTING DATE".to_string()))
Review Comment:
does this change the behavior with invalid dates (.e.g it will error rather than successfully write out "ERROR CONVERTING DATE")?
##########
arrow-csv/src/writer.rs:
##########
@@ -463,6 +384,19 @@ impl WriterBuilder {
self
}
+ /// Use RFC3339 format for date/time/timestamps by clearing all
Review Comment:
👍
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
JayjeetAtGithub commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1386626301
@alamb @tustvold I have done some initial changes here. Looking forward to your feedback. Thanks a lot.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1400340742
>
Hi @alamb , Looks like there is a conflict. Any ideas to handle this would be great. Thanks.
Sure @JayjeetAtGithub -- I'll try and work on it later today
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1413802968
Thanks again @JayjeetAtGithub and @tustvold !
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1091505911
##########
arrow-cast/src/display.rs:
##########
@@ -524,6 +721,22 @@ pub fn array_value_to_string(
}
}
+pub fn temporal_array_value_to_string(
Review Comment:
@alamb Sure. I was also thinking renaming this function to something like `array_value_to_string_using_format`. What do you think ?
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1401062207
Hi @JayjeetAtGithub
```shell
alamb@MacBook-Pro-8 arrow-rs % git fetch apache
git fetch apache
remote: Enumerating objects: 11, done.
remote: Counting objects: 100% (11/11), done.
remote: Compressing objects: 100% (9/9), done.
remote: Total 11 (delta 3), reused 2 (delta 2), pack-reused 0
Unpacking objects: 100% (11/11), 8.37 KiB | 779.00 KiB/s, done.
From github.com:apache/arrow-rs
de381ec54..b826657ac master -> apache/master
alamb@MacBook-Pro-8 arrow-rs % git merge apache/master
git merge apache/master
Auto-merging arrow-cast/src/display.rs
Auto-merging arrow-csv/src/writer.rs
CONFLICT (content): Merge conflict in arrow-csv/src/writer.rs
Automatic merge failed; fix conflicts and then commit the result.
```
I believe the logical conflict came from https://github.com/apache/arrow-rs/commit/24e5daef3248c38a6fb354c8427c9ba653e2b3e9 / https://github.com/apache/arrow-rs/pull/3570
Your changes may actually supercede the changes in https://github.com/apache/arrow-rs/pull/3570 but I would need to study it more carefully
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1402331382
Thank you for sticking with this @JayjeetAtGithub
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1405490151
@alamb I combined the changes #3570 and here is an initial version of that. I have mostly introduced a `col_idx` parameter to `temporal_array_value_to_string`, so that we can show an informative error message during a cast error. The `col_idx` parameter is an useful addition to not only temporal data types but all types. It requires a lot of changes, so If the current design looks good, we can do it as future work.
--
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
[GitHub] [arrow-rs] tustvold merged pull request #3514: Use array_value_to_string in arrow-csv
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1379938096
This functionality was added in https://github.com/apache/arrow-rs/pull/1347 by @gsserge with the original issue raised by @rjnanderson I'm a little concerned we are removing functionality here on which people depend.
I wonder if we can provide a way to pass options to array_value_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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] JayjeetAtGithub commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
JayjeetAtGithub commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1381209988
Thanks for the idea @tustvold @alamb. I will explore how to do that.
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1091827266
##########
arrow-cast/src/display.rs:
##########
@@ -133,57 +140,176 @@ macro_rules! make_string_interval_month_day_nano {
}
macro_rules! make_string_date {
- ($array_type:ty, $column: ident, $row: ident) => {{
- let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+ ($array_type:ty, $dt:expr, $column: ident, $col_idx:ident, $row_idx: ident) => {{
+ Ok($column
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .value_as_date($row_idx)
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .to_string())
+ }};
+}
- Ok(array
- .value_as_date($row)
- .map(|d| d.to_string())
- .unwrap_or_else(|| "ERROR CONVERTING DATE".to_string()))
Review Comment:
Yes, please, let's keep the existing behavior
I think https://github.com/apache/arrow-rs/issues/3638 will result in a way to eventually control what happens on invalid data (e.g. error or placeholder) so I think we should avoid changing the behavior 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1091826327
##########
arrow-cast/src/display.rs:
##########
@@ -524,6 +721,22 @@ pub fn array_value_to_string(
}
}
+pub fn temporal_array_value_to_string(
Review Comment:
if it is `pub` I think we should leave it the same name for now, as I suspect @tustvold has some larger changes in the API planed as part of https://github.com/apache/arrow-rs/issues/3638 and it would be nice to avoid requiring users to change their code twice
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#issuecomment-1401393496
Thanks a lot for taking a look at this @alamb.
--
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
[GitHub] [arrow-rs] JayjeetAtGithub commented on a diff in pull request #3514: Use array_value_to_string in arrow-csv
Posted by "JayjeetAtGithub (via GitHub)" <gi...@apache.org>.
JayjeetAtGithub commented on code in PR #3514:
URL: https://github.com/apache/arrow-rs/pull/3514#discussion_r1091496189
##########
arrow-cast/src/display.rs:
##########
@@ -133,57 +140,176 @@ macro_rules! make_string_interval_month_day_nano {
}
macro_rules! make_string_date {
- ($array_type:ty, $column: ident, $row: ident) => {{
- let array = $column.as_any().downcast_ref::<$array_type>().unwrap();
+ ($array_type:ty, $dt:expr, $column: ident, $col_idx:ident, $row_idx: ident) => {{
+ Ok($column
+ .as_any()
+ .downcast_ref::<$array_type>()
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .value_as_date($row_idx)
+ .ok_or_else(|| invalid_cast_error($dt, $col_idx, $row_idx))?
+ .to_string())
+ }};
+}
- Ok(array
- .value_as_date($row)
- .map(|d| d.to_string())
- .unwrap_or_else(|| "ERROR CONVERTING DATE".to_string()))
Review Comment:
Yeah, It would. Do we want to keep the former behaviour ?
--
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