GH-33143: [C++] Kernel to convert timestamp with timezone to wall time by rok · Pull Request #34208 · apache/arrow

rok · 2023-02-15T20:57:49Z

Rationale for this change

We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.

This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431

What changes are included in this PR?

This adds a local_time C++ kernel.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, local_time C++ kernel is added.

Closes: [C++] Kernel to convert timestamp with timezone to wall time #33143

github-actions · 2023-02-15T20:58:14Z

Closes: [C++] Kernel to convert timestamp with timezone to wall time #33143

westonpace

Thanks, as always, for your diligence here.

westonpace · 2023-02-17T01:03:01Z

docs/source/cpp/compute.rst

 +--------------------+------------+-------------------+---------------+----------------------------+-------+
 | is_leap_year       | Unary      | Timestamp, Date   | Boolean       |                            |       |
 +--------------------+------------+-------------------+---------------+----------------------------+-------+
+| local_time         | Unary      | Timestamp         | Timestamp     |                            |       |


Can we add a note? I don't think it will be obvious what this function does.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.

westonpace · 2023-02-17T01:05:21Z

cpp/src/arrow/compute/api_scalar.h

+/// \return the resulting datum
+///
+/// \since 12.0.0
+/// \note API not yet finalized


I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.

Well, you could see it as an opportunity to sync the API with substrait :D

rok · 2023-02-17T11:56:43Z

@github-actions crossbow submit preview-docs

github-actions · 2023-02-17T11:59:15Z

Revision: 115347e

Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef

Task	Status
preview-docs

rok · 2023-02-17T13:46:14Z

Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling

westonpace

Thanks, the description is clear to me. Two small grammar nits.

docs/source/cpp/compute.rst

Co-authored-by: Weston Pace <[email protected]>

rok · 2023-02-17T17:40:16Z

@github-actions crossbow submit preview-docs

rok · 2023-02-17T17:40:52Z

Thanks @westonpace, merged the grammar changes.

github-actions · 2023-02-17T17:42:41Z

Revision: cf2eb81

Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893

Task	Status
preview-docs

westonpace · 2023-02-17T20:11:45Z

The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).

ursabot · 2023-02-18T09:03:10Z

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] daa5e267 ec2-t3-xlarge-us-east-2
[Failed] daa5e267 test-mac-arm
[Finished] daa5e267 ursa-i9-9960x
[Finished] daa5e267 ursa-thinkcentre-m75q
[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2
[Finished] ae1f98f5 test-mac-arm
[Finished] ae1f98f5 ursa-i9-9960x
[Finished] ae1f98f5 ursa-thinkcentre-m75q
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

jorisvandenbossche · 2023-02-20T12:48:48Z

Nice to see this functionality!

One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?
(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).

For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.

jorisvandenbossche

And a few doc comments/questions

jorisvandenbossche · 2023-02-20T12:52:52Z

cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

-     "cannot be found in the timezone database."),
+     "If timezone is not given then timezone naive timestamp in UTC are\n"
+     "returned. An error is returned if the values have a defined timezone\n"
+     "but it cannot be found in the timezone database."),


Was this change intentional? Or was it meant to be in local_time_doc?
(and if intentional, why only for round and not ceil/floor?)

jorisvandenbossche · 2023-02-20T12:54:29Z

cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc


+const FunctionDoc local_time_doc{
+    "Convert timestamp to a timezone-naive local time timestamp",
+    ("LocalTime converts a timestamp to a local time of timestamps timezone\n"


Something is missing in this sentence?

jorisvandenbossche · 2023-02-20T12:54:53Z

cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

+     "and removes timezone metadata. If input is in UTC or doesn't have\n"
+     "timezone metadata, it is returned as is.\n"


Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?

rok · 2023-02-20T16:44:09Z

Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.

…rnel (#34263) ### Rationale for this change A better naming for local_time kernel was proposed in post merge review of #34208. ### What changes are included in this PR? Change `local_time` to `local_timestamp` and related docs/test changes. ### Are these changes tested? Yes. ### Are there any user-facing changes? Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking. * Closes: #33143 Authored-by: Rok Mihevc <[email protected]> Signed-off-by: Rok Mihevc <[email protected]>

github-actions bot added Component: C++ Component: Documentation Component: Python labels Feb 15, 2023

rok force-pushed the 33143-get-walltime branch 2 times, most recently from cdcb8c6 to 4a3bded Compare February 16, 2023 12:30

rok added 2 commits February 16, 2023 15:54

Adding local_time kernel

8857c38

More test cases. Python tests.

a5185a2

rok force-pushed the 33143-get-walltime branch from 4a3bded to a5185a2 Compare February 16, 2023 16:39

rok marked this pull request as ready for review February 16, 2023 17:11

rok requested review from AlenkaF and westonpace as code owners February 16, 2023 17:11

rok requested a review from lidavidm February 16, 2023 17:12

westonpace approved these changes Feb 17, 2023

View reviewed changes

Review feedback

115347e

westonpace approved these changes Feb 17, 2023

View reviewed changes

docs/source/cpp/compute.rst Outdated Show resolved Hide resolved

docs/source/cpp/compute.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

cf2eb81

Co-authored-by: Weston Pace <[email protected]>

westonpace merged commit daa5e26 into apache:main Feb 17, 2023

jorisvandenbossche reviewed Feb 20, 2023

View reviewed changes

rok mentioned this pull request Feb 20, 2023

GH-33143: [C++] Naming and doc/test changes for local_time compute kernel #34263

Merged

rok mentioned this pull request Dec 18, 2023

MINOR: [Docs] local_timestamp kernel docs are not linked in python docs #39274

Merged

		"and removes timezone metadata. If input is in UTC or doesn't have\n"
		"timezone metadata, it is returned as is.\n"

Conversation

rok commented Feb 15, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Feb 15, 2023

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

westonpace Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

rok Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

westonpace Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

rok Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

rok commented Feb 17, 2023

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

rok commented Feb 17, 2023

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rok commented Feb 17, 2023

Uh oh!

rok commented Feb 17, 2023

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

westonpace commented Feb 17, 2023

Uh oh!

ursabot commented Feb 18, 2023

Uh oh!

jorisvandenbossche commented Feb 20, 2023

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

rok commented Feb 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rok commented Feb 15, 2023 •

edited by github-actions bot

Loading