Account for Timezone when Casting Timestamp to Date32 #5605

Lordworms · 2024-04-08T13:26:47Z

Which issue does this PR close?

Closes #5598

Rationale for this change

consider time zone when casting timestamp -> Date32

What changes are included in this PR?

Are there any user-facing changes?

tustvold

I think this needs to make use of chrono, i.e. using DateTime<Tz> in order to be correct

tustvold · 2024-04-08T18:38:55Z

arrow-array/src/array/primitive_array.rs

-        self.with_timezone_opt(Some(timezone.into()))
+        let timezone_str = timezone.into().to_lowercase();
+
+        if timezone_str == "utc" {


Is this necessary?

I need this to help transfer timezone str to corresponding Tz

tustvold · 2024-04-08T18:39:18Z

arrow-array/src/timezone.rs

-
+    impl Tz {
+        /// get timezone
+        pub fn get_time_zone(&self) -> FixedOffset {


This will misbehave if the chrono-tz feature is enabled and we support timezones with DST

Sure, I'll change it

tustvold · 2024-04-12T08:08:03Z

arrow-array/src/array/primitive_array.rs

@@ -1366,7 +1366,12 @@ impl<T: ArrowTimestampType> PrimitiveArray<T> {

    /// Construct a timestamp array with new timezone
    pub fn with_timezone(self, timezone: impl Into<Arc<str>>) -> Self {
-        self.with_timezone_opt(Some(timezone.into()))
+        let timezone_arc = timezone.into();
+        if timezone_arc.eq_ignore_ascii_case("utc") {


This should be unnecessary

tustvold · 2024-04-12T08:09:04Z

arrow-array/src/timezone.rs

+        pub fn get_time_zone_min(&self) -> i32 {
+            match self.0 {
+                TzInner::Timezone(tz) => {
+                    let utc_datetime: DateTime<Utc> = Utc::now();


This is incorrect, the timezone must be computed with reference to the time in question. Perhaps you could look at the other temporal cast kernels for inspiration

Sure, I was just looking for a function to convert the timezone to int32/64

liukun4515

I think the fix is not right, we should not follow the timezone in the datatype to convert the value of date32.

We need to add the timezone config in the CastOptions which can't be configured by other system like datafusion.

liukun4515 · 2024-04-13T12:49:04Z

The value of timezone in the datatype of timestamp means the value of timestamp is a reference to the UTC/UNIX time.
https://github.com/apache/arrow/blob/main/format/Schema.fbs#L283

tustvold · 2024-04-13T12:54:17Z

We need to add the timezone config in the CastOptions which can't be configured by other system like datafusion.

The conversion should take the date of the timestamp in the timezone configured on the timestamp data type, we should not need to configure this on CastOptions, we have very carefully avoided introducing the notion of a "local" timezone.

So for example if we had 2019-10-12T07:20:50.52 +10:00, the date returned would be 2019-10-12 even though with reference to the UTC epoch (which is what the timestamp integer encodes) the date would be 2019-10-11.

Lordworms · 2024-04-14T18:24:16Z

@tustvold Hi, I have refactored the code to use DateTime, please take a look when you have time. Thank you!

tustvold

Thank you for this, I took the liberty of refactoring it slightly and adding a little more test coverage. Nicely done

liukun4515 · 2024-04-16T02:54:22Z

We need to add the timezone config in the CastOptions which can't be configured by other system like datafusion.

The conversion should take the date of the timestamp in the timezone configured on the timestamp data type, we should not need to configure this on CastOptions, we have very carefully avoided introducing the notion of a "local" timezone.

So for example if we had 2019-10-12T07:20:50.52 +10:00, the date returned would be 2019-10-12 even though with reference to the UTC epoch (which is what the timestamp integer encodes) the date would be 2019-10-11.

Hi @tustvold
From the definition of the timestamp in the arrow schema: https://github.com/apache/arrow/blob/main/format/Schema.fbs#L283

It's will cause inconvenient usage, let me give an example:

When I write a parquet file with timestamp column using the spark engine, the timestamp column is encoded by https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#instant-semantics-timestamps-normalized-to-utc and is normalized to UTC.

But when I use arrow-parquet to read the timestamp column, i will just get the column with timestamp(unit, "UTC") data type by this code

arrow-rs/parquet/src/arrow/schema/primitive.rs

Line 86 in 0d031cc

fn from_parquet(parquet_type: &Type) -> Result<DataType> {

and

arrow-rs/parquet/src/arrow/schema/primitive.rs

Line 232 in 0d031cc

(None, ConvertedType::TIMESTAMP_MILLIS) => Ok(DataType::Timestamp(

It's even possible to use int96 to store the timestamp in the parquet file, we will get the column with timestamp(nanosecond, None) data type by

arrow-rs/parquet/src/arrow/schema/primitive.rs

Line 99 in 0d031cc

PhysicalType::INT96 => Ok(DataType::Timestamp(TimeUnit::Nanosecond, None)),

But i need this to used in the UTC-7 timezone, how did you resolve this issue? @alamb

tustvold · 2024-04-16T06:15:53Z

You will need to explicitly cast the timezone back, unfortunately as parquet has no mechanism for encoding the timezone this will need to be done manually. Arrow writers embed the arrow schema allowing this to be done automatically, but Spark doesn't do this

alamb · 2024-04-16T15:42:14Z

Maybe we can add some way to the parquet arrow reader to override its choice of data type for certain columns to allow users to specify types for cases where it is not clear from the parquet file itself.

@mapleFU and I have been discussing the need for something similar for deciding what Array type to use when reading strings from Parquet files on #5530 -- see #5530 (comment))

If we had such an API then people using spark created parquet files could specify that the timestamp column should always be UTC (as suggested by @tustvold ) without having to add an explicit cast afterwards

liukun4515 · 2024-04-17T10:22:08Z

Maybe we can add some way to the parquet arrow reader to override its choice of data type for certain columns to allow users to specify types for cases where it is not clear from the parquet file itself.

@mapleFU and I have been discussing the need for something similar for deciding what Array type to use when reading strings from Parquet files on #5530 -- see #5530 (comment))

If we had such an API then people using spark created parquet files could specify that the timestamp column should always be UTC (as suggested by @tustvold ) without having to add an explicit cast afterwards

I think it's a solution adding the option in the parquet reader, it can help us to resolve some issues.

But many issues can't be resolve smoothly.

As describe in the comment: apache/datafusion#9981 (comment)
The same timestamp column maybe used by different timezone user, they may want to do compute on the same timestamp with different timezone. How to do that? @tustvold Do we need to add or wrap the cast expr explicitly to the target timestamp column?

tustvold · 2024-04-17T10:25:04Z

Do we need to add or wrap the cast expr explicitly to the target timestamp column?

Yes arrow does not have a notion of "local" timezone, users need to be explicit about the timezone they wish to use for a particular array. Theoretically a DF frontend could add such a notion, but I would strongly recommend against it as it is leads to all sorts of peculiar bugs.

The following would be an example DataFusion SQL query that is explicit about timezones

SELECT arrow_cast(time_column, 'Timestamp(Nanosecond, Some("+07:00"))';

liukun4515 · 2024-04-17T10:40:47Z

Do we need to add or wrap the cast expr explicitly to the target timestamp column?

Yes arrow does not have a notion of "local" timezone, users need to be explicit about the timezone they wish to use for a particular array. Theoretically a DF frontend could add such a notion, but I would strongly recommend against it as it is leads to all sorts of peculiar bugs.

The following would be an example DataFusion SQL query that is explicit about timezones
SELECT arrow_cast(time_column, 'Timestamp(Nanosecond, Some("+07:00"))';

Thanks for your feedback for

Do we need to add or wrap the cast expr explicitly to the target timestamp column?
I also know that adding explicit cast expr is not a good way.

Maybe the suggestion from @alamb is a good solution to resolve this after thinking about it again.
We can specify the timezone which is defined from parquet reader options when read the timestamp column.
what do you think about this method?

tustvold · 2024-04-17T10:51:13Z

Filed #5657

github-actions bot added the arrow Changes to the arrow crate label Apr 8, 2024

Lordworms mentioned this pull request Apr 8, 2024

confused result if ignore the timezone when cast timestamp to the date32 apache/datafusion#9982

Closed

tustvold reviewed Apr 8, 2024

View reviewed changes

Lordworms force-pushed the tz_cast_timestamp branch from 0cae5a2 to 026c03a Compare April 9, 2024 19:10

Lordworms requested a review from tustvold April 9, 2024 19:13

tustvold requested changes Apr 12, 2024

View reviewed changes

liukun4515 requested changes Apr 13, 2024

View reviewed changes

Consider Timezone when converting Timestamp to Date32

e9f99c7

Lordworms force-pushed the tz_cast_timestamp branch from 58dfbb3 to e9f99c7 Compare April 14, 2024 18:23

tustvold added 2 commits April 15, 2024 11:36

Remove UTC special case

df9486e

Cleanup

e6fe358

tustvold approved these changes Apr 15, 2024

View reviewed changes

Update tests

732495f

tustvold changed the title ~~adding timezone when casting timestamp~~ Account for Timezone when Casting Timestamp to Date32 Apr 15, 2024

tustvold merged commit 89767cc into apache:master Apr 15, 2024
26 checks passed

liukun4515 mentioned this pull request Apr 16, 2024

get error value if timestamp represented by the INT96 in the parquet file apache/datafusion#9981

Open

tustvold mentioned this pull request Apr 17, 2024

Provide Arrow Schema Hint to Parquet Reader #5657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for Timezone when Casting Timestamp to Date32 #5605

Account for Timezone when Casting Timestamp to Date32 #5605

Lordworms commented Apr 8, 2024 •

edited

Loading

tustvold left a comment

tustvold Apr 8, 2024

Lordworms Apr 8, 2024

tustvold Apr 8, 2024

Lordworms Apr 8, 2024

tustvold Apr 12, 2024

tustvold Apr 12, 2024

Lordworms Apr 13, 2024

liukun4515 left a comment

liukun4515 commented Apr 13, 2024

tustvold commented Apr 13, 2024 •

edited

Loading

Lordworms commented Apr 14, 2024

tustvold left a comment

liukun4515 commented Apr 16, 2024 •

edited

Loading

tustvold commented Apr 16, 2024

alamb commented Apr 16, 2024

liukun4515 commented Apr 17, 2024

tustvold commented Apr 17, 2024 •

edited

Loading

liukun4515 commented Apr 17, 2024

tustvold commented Apr 17, 2024

Account for Timezone when Casting Timestamp to Date32 #5605

Account for Timezone when Casting Timestamp to Date32 #5605

Conversation

Lordworms commented Apr 8, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

tustvold Apr 8, 2024

Choose a reason for hiding this comment

Lordworms Apr 8, 2024

Choose a reason for hiding this comment

tustvold Apr 8, 2024

Choose a reason for hiding this comment

Lordworms Apr 8, 2024

Choose a reason for hiding this comment

tustvold Apr 12, 2024

Choose a reason for hiding this comment

tustvold Apr 12, 2024

Choose a reason for hiding this comment

Lordworms Apr 13, 2024

Choose a reason for hiding this comment

liukun4515 left a comment

Choose a reason for hiding this comment

liukun4515 commented Apr 13, 2024

tustvold commented Apr 13, 2024 • edited Loading

Lordworms commented Apr 14, 2024

tustvold left a comment

Choose a reason for hiding this comment

liukun4515 commented Apr 16, 2024 • edited Loading

tustvold commented Apr 16, 2024

alamb commented Apr 16, 2024

liukun4515 commented Apr 17, 2024

tustvold commented Apr 17, 2024 • edited Loading

liukun4515 commented Apr 17, 2024

tustvold commented Apr 17, 2024

Lordworms commented Apr 8, 2024 •

edited

Loading

tustvold commented Apr 13, 2024 •

edited

Loading

liukun4515 commented Apr 16, 2024 •

edited

Loading

tustvold commented Apr 17, 2024 •

edited

Loading