Timezone aware timestamp parsing (#3794) #3795

tustvold · 2023-03-02T18:53:16Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

MichaelLeeHZ · 2023-03-03T07:41:01Z

arrow-cast/src/parse.rs

@@ -76,12 +42,14 @@ use chrono::prelude::*;
 ///     "2023-01-01 040506 +07:30:00",
 ///     "2023-01-01 04:05:06.789 PST",
 ///     "2023-01-01 04:05:06.789 -08",
-#[inline]
-pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
+pub fn string_to_datetime<T: TimeZone>(


There is a question here:
if I want to parse a string with time zone, what should I do with string_to_datetime? Do I have to use string_to_timestamp_nanos ?
The precondition is I do not know the format of timestamp-like string.

I don't fully understand this question

I would think you could call string_to_datetime<Utc>(...) which would give you a chrono DateTime<Utc> and you can then then do whatever you want with the result (e.g. convert it into a nanosecond timestamp, etc)

MichaelLeeHZ · 2023-03-03T11:36:32Z

arrow-cast/src/parse.rs

+        let utc = date.naive_utc().to_string();
+        assert_eq!(utc, "2020-09-08 13:42:29");
+        let local = date.naive_local().to_string();
+        assert_eq!(local, "2020-09-08 15:42:29");


Add some tests to check timestamp is correct?

Suggested change

assert_eq!(local, "2020-09-08 15:42:29");

assert_eq!(local, "2020-09-08 15:42:29");

let &tz = Local::now().offset();

let date = string_to_datetime(&tz, "2020-09-08T13:42:29Z").unwrap();

let dt = NaiveDateTime::parse_from_str("2020-09-08T13:42:29Z","%Y-%m-%dT%H:%M:%SZ").unwrap();

let ts = date.timestamp();

assert_eq!(dt.timestamp(), ts);

let date = string_to_datetime(&tz, "2020-09-08 13:42:29").unwrap();

assert_eq!(dt.timestamp() - (tz.local_minus_utc() as i64), date.timestamp());

I wanted to avoid using Local::now in the tests as this makes the tests not reproducible across machines. The tests above should cover this, no?

I guess the tests above can run ok across machines, you can try.

They do, but they don't consistently test the behaviour. For example my machine (and CI) is set to UTC, which means it won't test the behaviour at all...

Maybe we can use two different time zone to test the behavior. For example:

let tz1: Tz = "+02:00".parse().unwrap(); let tz2: Tz = "+08:00".parse().unwrap();

I added a further test, let me know if that isn't what you meant

That is OK, thx.

alamb

This makes sense to me -- thank you @tustvold and thank you @MachaelLee for bringing this issue up.

I wonder if you could test that this change works for your project's usecase somehow?

alamb · 2023-03-03T20:48:27Z

arrow-cast/src/parse.rs

@@ -76,12 +42,14 @@ use chrono::prelude::*;
 ///     "2023-01-01 040506 +07:30:00",
 ///     "2023-01-01 04:05:06.789 PST",
 ///     "2023-01-01 04:05:06.789 -08",
-#[inline]
-pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
+pub fn string_to_datetime<T: TimeZone>(


I don't fully understand this question

I would think you could call string_to_datetime<Utc>(...) which would give you a chrono DateTime<Utc> and you can then then do whatever you want with the result (e.g. convert it into a nanosecond timestamp, etc)

alamb · 2023-03-03T20:50:06Z

arrow-cast/src/parse.rs

@@ -112,34 +80,44 @@ pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
    // without a timezone specifier as a local time, using T as a separator
    // Example: 2020-09-08T13:42:29.190855
    if let Ok(ts) = NaiveDateTime::parse_from_str(s, "%Y-%m-%dT%H:%M:%S%.f") {
-        return to_timestamp_nanos(ts);
+        if let Some(offset) = timezone.offset_from_local_datetime(&ts).single() {


: https://docs.rs/chrono-tz/0.8.1/chrono_tz/enum.Tz.html#method.offset_from_local_datetime

alamb · 2023-03-03T20:53:23Z

arrow-cast/src/parse.rs

+/// For example, both `1997-01-31 09:26:56.123Z`, `1997-01-31T09:26:56.123`,
+/// and `1997-01-31T14:26:56.123-05:00` will be parsed as the same value
+///


I think some context got lost -- I only think this statement is true if the system timezone is set to UTC - 5:

Suggested change

/// For example, both `1997-01-31 09:26:56.123Z`, `1997-01-31T09:26:56.123`,

/// and `1997-01-31T14:26:56.123-05:00` will be parsed as the same value

///

/// For example, `1997-01-31 09:26:56.123Z`, `1997-01-31T09:26:56.123`,

/// and `1997-01-31T14:26:56.123-05:00` will be parsed as the same value

/// if the system timezone is set to Americas/New_York (UTC-5).

Nope, that context was actually incorrect. They will always parse as the same regardless of the local timezone. If you want local timezone specific behaviour you should pass Local to the function above

arrow-cast/src/parse.rs

alamb · 2023-03-03T20:54:02Z

arrow-cast/src/parse.rs

+        let dt =
+            NaiveDateTime::parse_from_str("2020-09-08T13:42:29Z", "%Y-%m-%dT%H:%M:%SZ")
+                .unwrap();
+        let local: Tz = "+08:00".parse().unwrap();


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

ursabot · 2023-03-04T11:21:41Z

Benchmark runs are scheduled for baseline = 7fdd0d8 and contender = 6cd0917. 6cd0917 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

github-actions bot added the arrow Changes to the arrow crate label Mar 2, 2023

Timezone aware timestamp parsing (apache#3794)

e621c84

tustvold force-pushed the timezone-aware-string-parsing branch from d878113 to e621c84 Compare March 2, 2023 18:53

tustvold mentioned this pull request Mar 2, 2023

feat: add a function to parse a string without timezone to local timestamp #3787

Closed

MichaelLeeHZ reviewed Mar 3, 2023

View reviewed changes

Add further test

aab85b3

tustvold mentioned this pull request Mar 3, 2023

Faster timestamp parsing (~70-90% faster) #3801

Merged

alamb approved these changes Mar 3, 2023

View reviewed changes

alamb mentioned this pull request Mar 3, 2023

Timezone Aware Timestamp Parsing #3794

Closed

Update arrow-cast/src/parse.rs

55221bf

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

tustvold merged commit 6cd0917 into apache:master Mar 4, 2023

This was referenced Mar 10, 2023

CSV Reader Doesn't set Timezone #3841

Closed

Specific timezone support for to_timetamp*() apache/datafusion#686

Open

Timestamp subtraction and interval operations for ScalarValue apache/datafusion#5603

Merged

tustvold mentioned this pull request Apr 1, 2023

Timestamps having different timezone cannot be represented in the same field of a schema apache/datafusion#5823

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timezone aware timestamp parsing (#3794) #3795

Timezone aware timestamp parsing (#3794) #3795

tustvold commented Mar 2, 2023

MichaelLeeHZ Mar 3, 2023

alamb Mar 3, 2023

MichaelLeeHZ Mar 3, 2023

tustvold Mar 3, 2023 •

edited

Loading

MichaelLeeHZ Mar 3, 2023

tustvold Mar 3, 2023

MichaelLeeHZ Mar 3, 2023

tustvold Mar 3, 2023

MichaelLeeHZ Mar 4, 2023

alamb left a comment

alamb Mar 3, 2023

alamb Mar 3, 2023

alamb Mar 3, 2023

tustvold Mar 3, 2023 •

edited

Loading

alamb Mar 3, 2023

ursabot commented Mar 4, 2023

-        assert_eq!(local, "2020-09-08 15:42:29");
+        assert_eq!(local, "2020-09-08 15:42:29");
+        let &tz = Local::now().offset();
+        let date = string_to_datetime(&tz, "2020-09-08T13:42:29Z").unwrap();
+        let dt = NaiveDateTime::parse_from_str("2020-09-08T13:42:29Z","%Y-%m-%dT%H:%M:%SZ").unwrap();
+        let ts = date.timestamp();
+        assert_eq!(dt.timestamp(), ts);
+        let date = string_to_datetime(&tz, "2020-09-08 13:42:29").unwrap();
+        assert_eq!(dt.timestamp() - (tz.local_minus_utc() as i64), date.timestamp());

Timezone aware timestamp parsing (#3794) #3795

Timezone aware timestamp parsing (#3794) #3795

Conversation

tustvold commented Mar 2, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Mar 3, 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

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

tustvold Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Mar 4, 2023

tustvold Mar 3, 2023 •

edited

Loading

tustvold Mar 3, 2023 •

edited

Loading