ARROW-13033: [C++] Kernel to localize naive timestamps to a timezone (preserving clock-time)

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !

[rok]

This is to resolve ARROW-13033.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13033

[jorisvandenbossche]

Given all the discussion on the JIRA issue, can you a bit more clearly state what exactly you implemented and what part it covers?
For example, the current kernel returns int64 and not timestamp with timezone?

It also seems that the implementation is converting system time (UTC epoch) to local (naive) time, and not the other way around as the docstring says.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

[rok]

Thanks for the review @jorisvandenbossche :).
I've gone through your feedback and done some of the boilerplate except for the correct output timestamp type. I'll try to do it over the weekend.

(also, given all the confusion also on the JIRA, we should probably try to come up with a more explicit/descriptive name ...)

I think there is no consensus on this yet. tz_convert, tz_localize were mentioned. I'd add tz_normalize. Any other?

[jorisvandenbossche]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

[rok]

@rok can you explain the rationale of adding a source_timezone and destination_timezone to the API? What's the reason for doing it like this, instead of having a single target timezone as argument?

In case we start with "naive" a timestamp we don't know the source and target timezone so both need to be specified. Or am I missing something here?

[jorisvandenbossche]

What's the difference between both timezones? In my mind this is the same time zone? If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

[rok]

If you want to convert your localized timezone-aware timestamp to another timezone after localizing, you can use another kernel to do that? (a kernel we don't have yet, but one we should also add, although this is a trivial, metadata-only change)

I was thinking only of the metadata change not "materialization" of a target timezone. Having another kernel to only change metadata seems redundant.

[jorisvandenbossche]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

[jorisvandenbossche]

(if we keep both source and destination timezone in the "localize" kernel, I think at least the destination timezone, if not specified, should default to be the same as the source timezone, and not "UTC", so that a user doesn't have to specify the timezone they want twice, like localize(timestamp, source_timezone="Europe/Brussels", destination_timezone="Europe/Brussels") to end up with a timestamp(unit, tz="Europe/Brussels) type column)

[jorisvandenbossche]

I opened https://issues.apache.org/jira/browse/ARROW-13247 for the discussion whether we want a separate "change timezone" kernel

[rok]

Having another kernel to only change metadata seems redundant.

I don't think that's redundant, as a user will regularly want to change the timezone of a timestamp column that is already localized (already has a timezone).

I am of course coming from the point of view of pandas, where those two operations are defined as two distinct methods: tz_localize to convert from "timestamp without timezone" to "timestamp with timezone" (i.e. from naive clock time to tz-aware time) and tz_convert to convert between "timestamp with timezone" with different timezones (a metadata-only change).
From a user point of view (and also implementation-wise), those two are distinct concepts, IMO, and so I would keep them as separate kernels. (also eg R's lubridate has distinct force_tz and with_tz, respectively)

I suppose these are two different operations indeed. I've changed this to only do "localization" now.

I like force_tz as it indicates the operation is a bit "dangerous". Candidates are then:

• force_tz
• tz_localize
• tz_normalize

[adamhooper]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

[rok]

Since the Arrow spec is going to refer to "LocalDateTime", "Instant" and "ZonedDateTime", how about reusing terms from the spec? Something along the lines of local_datetime_to_zoned_datetime()?

It's nice that it's idiomatic if a bit verbose :)

Let's add it to the list. If we don't get to consensus here we can always call a vote on the ML.

• force_tz
• tz_localize
• tz_normalize
• local_datetime_to_zoned_datetime

[westonpace]

I like all except tz_normalize. "normalize" just has too many meanings for me already.

[jorisvandenbossche]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

And from testing, you will still need to handle the errors raised by date.h:

In [21]: arr = pa.array([pd.Timestamp("2021-03-28 02:30:00")], type=pa.timestamp("ns"))
In [22]: pc.localize(arr, options=pc.TemporalLocalizationOptions("Europe/Brussels"))
terminate called after throwing an instance of 'arrow_vendored::date::nonexistent_local_time'
what(): 2021-03-28 02:30:00.000000000 is in a gap between
2021-03-28 02:00:00 CET and
2021-03-28 03:00:00 CEST which are both equivalent to
2021-03-28 01:00:00 UTC
Aborted (core dumped)

[rok]

@rok I was checking out this PR, and in the process wrote python bindings for the option class to be able to test it in Python, so thought to directly push that here as well. (and at the same time also fixed (I think) the initialization of the timezone, which was causing the C++ tests to fail here as well)

Thanks @jorisvandenbossche! The new option initialization was indeed problematic.

[pitrou]

I'm going to suggest assume_timezone.

[rok]

What would be a good way to reach consensus here? A doodle poll to ML to see what people find most intuitive?

[pitrou]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You may still ask for opinions on the ML, but I would personally go with assume_timezone. Users will have to read the documentation in any case.

[rok]

Well, I don't think anything is intuitive here, because the semantics of timestamps in Arrow are slightly weird and surprising :-)

You mean timestamps and timezones in general are weird (because the underlying problem is counter intuitive)? If not we should really make this as simple as the underlying complexity is.

[rok]

@pitrou I've changed this to assume_timezone (AssumeTimezone, AssumeTimezoneOptions).
Discussion on Zulip seems to be in favor of assume_timestamp.

Would still be good to hear @jorisvandenbossche@adamhooper@westonpace

[adamhooper]

@rok Thanks for asking! I prefer assume_timezone, too.

[pitrou]

I don't think this should be here. Is it deliberate or just a leftover?

[rok]

Oh. Probably a bad rebase.

[rok]

Removed.

[pitrou]

I don't really understand what this is for. Presumably this should be unreachable?

[rok]

I think it was just to satisfy the compiler.

[rok]

Removed the status but kept the return 0 to keep he compiler happy. Please check.

[rok]

CI fail seems unrelated.

[pitrou]

Ok, I've pushed some minor changes and will merge once CI is green. Thanks a lot for this @rok !

[rok]

Let me rebase that.

[pitrou]

@rok I'm on it already.

[rok]

Thanks @jorisvandenbossche@pitrou@adamhooper@westonpace !