Fix csv writing of timestamps to show timezone. #849

novemberkilo · 2021-10-23T05:14:30Z

Which issue does this PR close?

Closes #778

What changes are included in this PR?

Round-trip test of csv writing and reading of timestamps
Fix to csv writer so that timestamps are written out in UTC.

Are there any user-facing changes?

Yes. This changes the behaviour of the csv writer.

// @alamb @bjchambers

codecov-commenter · 2021-10-23T05:32:11Z

Codecov Report

Attention: Patch coverage is 23.25581% with 33 lines in your changes missing coverage. Please review.

Project coverage is 82.28%. Comparing base (898924f) to head (e83b111).
Report is 2721 commits behind head on master.

Files	Patch %	Lines
arrow/src/csv/writer.rs	24.39%	31 Missing ⚠️
arrow/src/compute/kernels/temporal.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   82.45%   82.28%   -0.17%     
==========================================
  Files         168      168              
  Lines       48231    48575     +344     
==========================================
+ Hits        39767    39971     +204     
- Misses       8464     8604     +140

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alamb

Thanks @novemberkilo !

arrow/src/csv/writer.rs

novemberkilo · 2021-10-31T06:58:58Z

@alamb @bjchambers I have made changes to the csv writer according to the proposal.

While doing so, I have noticed what I think is a bug in the temporal utilities in the kernel. Essentially, variations relating to daylight savings are not recognised at the moment. I have a failing test on a branch here. I came up across this issue during this work as and had to implement https://github.com/novemberkilo/arrow-rs/blob/master/arrow/src/compute/kernels/temporal.rs#L98-L110 to correctly derive the offset for a timezone.

I figure I should fix this bug separately -- should I file an issue to describe it and then put up a fix against that? Thanks.

alamb · 2021-11-02T11:21:35Z

I figure I should fix this bug separately -- should I file an issue to describe it and then put up a fix against that? Thanks.

I think that would make the most sense to me @novemberkilo -- thanks

alamb · 2021-11-09T11:05:34Z

Thanks @novemberkilo -- I plan to review this more carefully hopefully today

alamb

Thanks @novemberkilo this is looking quite nice.

arrow/src/compute/kernels/temporal.rs

arrow/src/csv/writer.rs

novemberkilo · 2021-11-13T12:00:40Z

Thanks much for the feedback @alamb -- I have reviewed your comments and made changes accordingly.

Something to note, instead of adding Z to indicate UTC, we now consistently use the same format string and show UTC as +00:00

alamb

Looks great -- thank you @novemberkilo

alamb · 2021-11-15T13:56:04Z

arrow/src/csv/writer.rs

+        let expected = vec![Some(3), Some(2), Some(1)];
+        assert_eq!(actual, expected);
+        let actual = c3.into_iter().collect::<Vec<_>>();
+        let expected = nanoseconds.into_iter().map(|x| Some(x)).collect::<Vec<_>>();


novemberkilo · 2021-11-15T19:51:04Z

Thanks @alamb - once this is merged I will put up a PR for #901 that builds from this

alamb · 2021-11-15T22:19:36Z

I'll plan to merge this tomorrow morning unless anyone else has any comments

alamb · 2021-11-16T11:11:28Z

Thanks again @novemberkilo

* Write timestamps (in csvs) with timezone. * More tests and more verbose naming. * Please linter. * Please clippy. * Cleanup based on review feedback.

* Write timestamps (in csvs) with timezone. * More tests and more verbose naming. * Please linter. * Please clippy. * Cleanup based on review feedback. Co-authored-by: Navin <[email protected]>

github-actions bot added the arrow Changes to the arrow crate label Oct 23, 2021

novemberkilo force-pushed the master branch from 672fca6 to 2f874dc Compare October 23, 2021 05:15

alamb reviewed Oct 23, 2021

View reviewed changes

arrow/src/csv/writer.rs Outdated Show resolved Hide resolved

novemberkilo force-pushed the master branch from 2f874dc to 2fa4058 Compare October 31, 2021 06:49

novemberkilo changed the title ~~Fix csv writing of timestamps to be in UTC.~~ Fix csv writing of timestamps to show timezone. Oct 31, 2021

Write timestamps (in csvs) with timezone.

2fa4058

novemberkilo mentioned this pull request Nov 1, 2021

Temporal utilities miss variations due to daylight savings #901

Closed

novemberkilo added 3 commits November 7, 2021 17:33

More tests and more verbose naming.

82e4c00

Please linter.

3b0652f

Please clippy.

1c1822b

alamb reviewed Nov 10, 2021

View reviewed changes

Cleanup based on review feedback.

e83b111

alamb approved these changes Nov 15, 2021

View reviewed changes

alamb merged commit bfc95be into apache:master Nov 16, 2021

alamb pushed a commit that referenced this pull request Nov 20, 2021

Fix csv writing of timestamps to show timezone. (#849)

2ae1ea6

* Write timestamps (in csvs) with timezone. * More tests and more verbose naming. * Please linter. * Please clippy. * Cleanup based on review feedback.

alamb added the cherry-picked label Nov 20, 2021

alamb mentioned this pull request Nov 20, 2021

Cherry pick Fix csv writing of timestamps to show timezone. to active_release #963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix csv writing of timestamps to show timezone. #849

Fix csv writing of timestamps to show timezone. #849

novemberkilo commented Oct 23, 2021

codecov-commenter commented Oct 23, 2021 •

edited

Loading

alamb left a comment

novemberkilo commented Oct 31, 2021

alamb commented Nov 2, 2021

alamb commented Nov 9, 2021

alamb left a comment

novemberkilo commented Nov 13, 2021

alamb left a comment

alamb Nov 15, 2021

novemberkilo commented Nov 15, 2021 •

edited

Loading

alamb commented Nov 15, 2021

alamb commented Nov 16, 2021

Fix csv writing of timestamps to show timezone. #849

Fix csv writing of timestamps to show timezone. #849

Conversation

novemberkilo commented Oct 23, 2021

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Oct 23, 2021 • edited Loading

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

novemberkilo commented Oct 31, 2021

alamb commented Nov 2, 2021

alamb commented Nov 9, 2021

alamb left a comment

Choose a reason for hiding this comment

novemberkilo commented Nov 13, 2021

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 15, 2021

Choose a reason for hiding this comment

novemberkilo commented Nov 15, 2021 • edited Loading

alamb commented Nov 15, 2021

alamb commented Nov 16, 2021

codecov-commenter commented Oct 23, 2021 •

edited

Loading

novemberkilo commented Nov 15, 2021 •

edited

Loading