Use array_value_to_string in arrow-csv #3514

JayjeetAtGithub · 2023-01-12T01:03:37Z

Which issue does this PR close?

Closes #3483.
Closes #3513

tustvold · 2023-01-12T07:56:21Z

This functionality was added in #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 🤔

alamb · 2023-01-12T21:42:49Z

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

JayjeetAtGithub · 2023-01-13T01:51:46Z

Thanks for the idea @tustvold @alamb. I will explore how to do that.

JayjeetAtGithub · 2023-01-18T07:51:59Z

@alamb @tustvold I have done some initial changes here. Looking forward to your feedback. Thanks a lot.

alamb · 2023-01-18T22:58:08Z

Thanks @JayjeetAtGithub -- I plan to take a look tomorrow

alamb

Thanks @JayjeetAtGithub -- it is really nice to get your contribution and I apologize in the delay of reviewing -- I think this is looking quite close.

The things I think are needed are:

An explanation about why test_conversion_consistency was removed
Change the signature from &Option<String> to Option<&str>

Nice to have

Add some tests showing the rfc3339 behavior for time, dates, datetime, timestamps, timestamptz

When this PR is merged I'll try and write up some notes about making a better display API but I don't want to confuse this PR any more

alamb · 2023-01-19T11:34:20Z

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


this sure looks better 😍

alamb · 2023-01-19T11:40:36Z

arrow-cast/src/display.rs

    column: &ArrayRef,
    row: usize,
+    datetime_format_opt: &Option<String>,


FYI I think the typical rust way to do this (and not require an owned String) is:

Suggested change

datetime_format_opt: &Option<String>,

datetime_format_opt: Option<&str>,

And then at callsites with val: Option<String> instead of:

array_value_to_string_internal(foo, bar, &val)

You do :

array_value_to_string_internal(foo, bar, val.as_ref())

alamb · 2023-01-19T11:41:03Z

arrow-csv/src/writer.rs

            .to_string(),
            String::from_utf8(buffer).unwrap()
        );
    }
-
-    #[test]
-    fn test_conversion_consistency() {


Why was this test removed? It seems useful, doesn't it?

alamb · 2023-01-19T11:42:56Z

arrow-csv/src/writer.rs

@@ -430,33 +364,45 @@ impl WriterBuilder {
        self
    }

+    /// Whether to use RFC3339 format for date/time/timestamps


Suggested change

/// Whether to use RFC3339 format for date/time/timestamps

/// Use RFC3339 format for date/time/timestamps by clearing all

/// date/time specific formats.

alamb · 2023-01-19T11:44:18Z

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;


Rather than an extra flag, what would you think about having this function simply set

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

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.

alamb · 2023-01-19T11:49:32Z

arrow-cast/src/display.rs

+    array_value_to_string_internal(column, row, &None)
+}
+
+pub fn datetime_array_value_to_string(


I am somewhat worried about adding a new public API like this, but I see it is needed to call in arrow-csv

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

alamb · 2023-01-19T11:49:43Z

arrow-cast/src/display.rs

+pub fn datetime_array_value_to_string(
+    column: &ArrayRef,
+    row: usize,
+    format: &Option<String>,


Suggested change

format: &Option<String>,

format: Option<&str>,

JayjeetAtGithub · 2023-01-22T02:07:37Z

Hi @alamb , Looks like there is a conflict. Any ideas to handle this would be great. Thanks.

alamb · 2023-01-23T13:28:56Z

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

alamb · 2023-01-23T22:08:27Z

Hi @JayjeetAtGithub

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 24e5dae / #3570

Your changes may actually supercede the changes in #3570 but I would need to study it more carefully

JayjeetAtGithub · 2023-01-24T04:50:07Z

Thanks a lot for taking a look at this @alamb.

alamb · 2023-01-24T17:35:14Z

Thank you for sticking with this @JayjeetAtGithub

JayjeetAtGithub · 2023-01-26T19:21:54Z

@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, I can add it in this PR. Thanks.

…ption<String>

alamb

Thanks @JayjeetAtGithub -- I plan to review this one carefully tomorrow

alamb

I think this looks good to go -- thank you @JayjeetAtGithub

I am slightly concerned that we may have changed behavior of array_value_to_string on invalid data (aka return an error rather than a string) but I don't see any tests that change 🤔

cc @viirya and @tustvold

alamb · 2023-01-30T13:54:53Z

arrow-cast/src/display.rs

+
+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}"


I like the addition of column index to the error message 👍

alamb · 2023-01-30T13:57:19Z

arrow-cast/src/display.rs

-        Ok(array
-            .value_as_date($row)
-            .map(|d| d.to_string())
-            .unwrap_or_else(|| "ERROR CONVERTING DATE".to_string()))


does this change the behavior with invalid dates (.e.g it will error rather than successfully write out "ERROR CONVERTING DATE")?

Yeah, It would. Do we want to keep the former behaviour ?

Yes, please, let's keep the existing behavior

I think #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

alamb · 2023-01-30T13:58:26Z

arrow-cast/src/display.rs

@@ -524,6 +721,22 @@ pub fn array_value_to_string(
    }
 }

+pub fn temporal_array_value_to_string(


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

@alamb Sure. I was also thinking renaming this function to something like array_value_to_string_using_format. What do you think ?

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 #3638 and it would be nice to avoid requiring users to change their code twice

alamb · 2023-01-30T13:58:53Z

arrow-csv/src/writer.rs

-                }
-                DataType::Date64 => {
-                    write_temporal_value!(
+                DataType::Boolean => array_value_to_string(col, row_index)?.to_string(),


👍 that certainly looks better

alamb · 2023-01-30T13:59:18Z

arrow-csv/src/writer.rs

@@ -463,6 +384,19 @@ impl WriterBuilder {
        self
    }

+    /// Use RFC3339 format for date/time/timestamps by clearing all


alamb · 2023-01-30T14:29:28Z

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: #3638

tustvold · 2023-02-02T13:18:57Z

I'm going to merge this in so #3647 doesn't generate huge numbers of conflicts with it

ursabot · 2023-02-02T13:21:43Z

Benchmark runs are scheduled for baseline = 2b9bbce and contender = 75a56be. 75a56be is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
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

alamb · 2023-02-02T14:07:55Z

Thanks again @JayjeetAtGithub and @tustvold !

github-actions bot added the arrow Changes to the arrow crate label Jan 12, 2023

JayjeetAtGithub mentioned this pull request Jan 12, 2023

Use array_value_to_string in arrow-csv #3483

Closed

tustvold added the api-change Changes to the arrow API label Jan 12, 2023

tustvold marked this pull request as draft January 13, 2023 07:49

JayjeetAtGithub marked this pull request as ready for review January 18, 2023 07:49

alamb reviewed Jan 19, 2023

View reviewed changes

JayjeetAtGithub requested a review from alamb January 21, 2023 21:57

JayjeetAtGithub force-pushed the issue-3483 branch from e97f8cb to ffbb030 Compare January 22, 2023 01:32

JayjeetAtGithub force-pushed the issue-3483 branch from ffbb030 to c36433a Compare January 25, 2023 20:09

JayjeetAtGithub added 10 commits January 26, 2023 21:58

Use array_value_to_string in arrow-csv

8ebc723

Fix test

5d89195

Add datetime_array_value_to_string to allow passing datetime format

20cf37a

Rollback the tests

5a2fb4d

Add option to use RFC3339 in CSV writeR

91cfb7f

Update tests

44aff40

Fix linting errors

779a681

fix tests

5414464

Change with_rfc3339 factory method and use Option<&str> instead of &O…

ee0c794

…ption<String>

Keep old tests intact

5d7a4ce

JayjeetAtGithub added 11 commits January 26, 2023 21:58

Add tests to check rfc3339

9a3ceef

Add back test_conversion_consistency

f3f3eba

Fix clippy errors

d476c27

Minor linting issue

83e897d

Separate array_value_to_string and datetime_array_value_to_string

5d7a603

Add back invalid cast test

cab2030

Fix linting and clippy errors

286bc52

Fix arrow-cast test

8b6e856

Restructuring

c50104a

Fix formatting errors

f75855c

Change make_duration_string to use invalid_cast_error

e52940a

JayjeetAtGithub force-pushed the issue-3483 branch from c2be153 to e52940a Compare January 27, 2023 06:00

Fix clippy errors

98251e1

JayjeetAtGithub force-pushed the issue-3483 branch from 7ad8a8b to 98251e1 Compare January 27, 2023 06:41

alamb reviewed Jan 29, 2023

View reviewed changes

alamb approved these changes Jan 30, 2023

View reviewed changes

alamb mentioned this pull request Jan 30, 2023

Array --> Display formatter that supports more options and is configurable #3638

Closed

4 tasks

tustvold merged commit 75a56be into apache:master Feb 2, 2023

comphead mentioned this pull request Feb 3, 2023

When the to_timestamp_millis function is used and the output format is csv, it will panic apache/datafusion#4947

Closed

This was referenced Feb 3, 2023

Use array_value_to_string when writing CSV, support RFC3339 style timestamps in arrow-json #3417

Closed

Inconsistent output between pretty print and CSV writer for Arrow #3513

Closed

JayjeetAtGithub deleted the issue-3483 branch September 1, 2023 02:38

	datetime_format_opt: &Option<String>,
	datetime_format_opt: Option<&str>,

	/// Whether to use RFC3339 format for date/time/timestamps
	/// Use RFC3339 format for date/time/timestamps by clearing all
	/// date/time specific formats.

Use array_value_to_string in arrow-csv #3514

Use array_value_to_string in arrow-csv #3514

Conversation

JayjeetAtGithub commented Jan 12, 2023 • edited by alamb Loading

Which issue does this PR close?

tustvold commented Jan 12, 2023

alamb commented Jan 12, 2023

JayjeetAtGithub commented Jan 13, 2023

JayjeetAtGithub commented Jan 18, 2023

alamb commented Jan 18, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayjeetAtGithub commented Jan 22, 2023

alamb commented Jan 23, 2023

alamb commented Jan 23, 2023

JayjeetAtGithub commented Jan 24, 2023

alamb commented Jan 24, 2023

JayjeetAtGithub commented Jan 26, 2023 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 30, 2023

tustvold commented Feb 2, 2023

ursabot commented Feb 2, 2023

alamb commented Feb 2, 2023

JayjeetAtGithub commented Jan 12, 2023 •

edited by alamb

Loading

alamb Jan 19, 2023 •

edited

Loading

JayjeetAtGithub commented Jan 26, 2023 •

edited

Loading