ARROW-13174: [C++][Compute] Add strftime kernel by rok · Pull Request #10647 · apache/arrow

rok · 2021-07-02T00:52:21Z

This is to resolve ARROW-13174.

github-actions · 2021-07-02T00:52:41Z

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

rok · 2021-07-09T22:40:48Z

@westonpace strftime kernel is almost ready for review.
I do need to take care of the string encoding.

westonpace

Thanks for doing this. Looks great.

westonpace · 2021-07-13T01:38:48Z

cpp/src/arrow/compute/api_scalar.cc

Should the default format string include the trailing %z so that it is ISO-8601 compliant?

Indeed. Will add.

I don't think that %z adds a trailing z as in the ISO format. It adds a numeric offset (eg https://www.cplusplus.com/reference/ctime/strftime/):

In [44]: datetime.datetime(2012, 1, 1, tzinfo=datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%S%z") Out[44]: '2012-01-01T00:00:00+0000'

According to wikipedia these are valid formats:

2021-07-15T11:24:54+00:00

2021-07-15T11:24:54Z

20210715T112454Z

%z will give us option 1. I don't have a strong preference and pandas doesn't seem to have a default.

If it is significant effort I agree that +00:00 is good enough but I personally prefer Z if there is a quick option to enable it when the time zone is UTC.

Switched to %Y-%m-%dT%H:%M:%SZ (2.) as default.

westonpace · 2021-07-13T01:46:49Z

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

Nit: How precise is this 30? Is it based on the default format string? If I look at 2021-07-12T23:55:41+00:00 I would think you only need ~25, maybe more if subsecond resolution. Could we base this on the resolution? It's not a big deal so if it seems like too much don't worry.

How about a random date's string size times a fudge factor? :)

int expected_string_size = round(get_timestamp<Duration>(0, &options).size() * 1.1); RETURN_NOT_OK(string_builder->Reserve(in.length * expected_string_size));

python/pyarrow/tests/test_compute.py

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

rok · 2021-07-13T14:35:10Z

Thanks for the review @westonpace!
I've gone through it and pushed appropriate changes.

One thing I'm not sure about is if we should be using date.h for formatting here or would it be better to use system / c++ equivalents.

pitrou · 2021-08-17T12:58:18Z

Yes, you can. Make it return a Result<const time_zone*>

rok · 2021-08-17T14:51:06Z

Yes, you can. Make it return a Result<const time_zone*>

@pitrou Done. Please review.

rok · 2021-08-17T14:54:36Z

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

Please note this is only testing for C locale because that is all that is available in CI at the moment. Ideally we would cover several.

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

pitrou · 2021-08-18T14:13:50Z

Will merge if green.

rok · 2021-08-18T16:09:17Z

Nice! Thanks @westonpace @jorisvandenbossche @pitrou

jorisvandenbossche · 2021-08-19T10:05:02Z

Sorry for the slow response here, but I think there are still a few behavioural aspects to fix/clarify:

Related to @westonpace's comment above (ARROW-13174: [C++][Compute] Add strftime kernel #10647 (comment)), you added a "Z" to the default format. However, this is only correct if you have a UTC timezone, and not for any other timezone. For example:
```
>>> ts = pd.to_datetime(["2018-03-10 09:00"]).tz_localize("US/Eastern")
>>> ts
DatetimeIndex(['2018-03-10 09:00:00-05:00'], dtype='datetime64[ns, US/Eastern]', freq=None)
>>> tsa = pa.array(ts)
>>> tsa
<pyarrow.lib.TimestampArray object at 0x7f7350b087c0>
[
  2018-03-10 14:00:00.000000000
]

>>> pc.strftime(tsa)
<pyarrow.lib.StringArray object at 0x7f7350b74a60>
[
  "2018-03-10T09:00:00.000000000Z"
]
```
So it's correctly showing the timestamp in the timezone's local time, but thus the "Z" indicator for UTC is wrong (the correct UTC time is 14:00, not 09:00).
I think we should only add the "Z" indicator if the timezone is UTC. I am not fully sure what we should then use as default format for non-UTC timezones though: don't show any timezone information, include a numeric offset, or error.
That would also mean that the "default" format string would depend on the input type of the data, which might not be easy / desirable.

I commented about the timezone handling when the initial PR had a keyword for this, but I forgot to reply after you removed that keyword (and support for local timestamps) altogether. But, what's the reasoning for disallowing local timestamps without timezone? I don't think there is any ambiguity in how they would be formatted? (after all, they represent "clock" time, which in the end is kind of a formatted string)
There was some discussion above about the behaviour of %S (ARROW-13174: [C++][Compute] Add strftime kernel #10647 (comment)), where date.h / C++ handles it differently as Python or R (i.e. we are including the fractional sub-second decimals, and there is no easy way to only show integer seconds apart from casting to timestamp("s") first AFAIK).
Since there are conflicting standards vs language implementations, there is no easy way to solve this. But I think it would be good to at least document this difference (it will be surprising for Python/R users) and how to work-around it.

rok · 2021-08-20T13:35:28Z

Thanks for the review @jorisvandenbossche !
I think we need to address these points - I'll open a follow-up JIRA and do some work in a week or so.

Indeed, current situation (Z after non UTC string) is wrong. I think your suggestion (check input timezone and append Z to default format if UTC) would be ok.
I forget what the thought was at the time. But looking at our discussion it might have been a date.h limitation. I'll look into it.
Agreed! We could also look into upstreaming a behavior flag to date.h but I think we'd need stronger motivation.

rok · 2021-08-20T13:42:43Z

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

jorisvandenbossche · 2021-08-23T10:00:39Z

Thanks!

rok · 2021-08-25T13:56:05Z

Follow up PR #10998

github-actions bot added the Component: C++ label Jul 2, 2021

rok force-pushed the ARROW-13174 branch 3 times, most recently from 2436df1 to 45acfcd Compare July 9, 2021 00:06

github-actions bot added Component: Python Component: R labels Jul 9, 2021

rok force-pushed the ARROW-13174 branch 7 times, most recently from 652d46d to 78d3ffc Compare July 9, 2021 22:31

rok force-pushed the ARROW-13174 branch 6 times, most recently from cfa23ee to 9e8b775 Compare July 12, 2021 23:44

rok marked this pull request as ready for review July 13, 2021 00:21

westonpace approved these changes Jul 13, 2021

View reviewed changes

rok force-pushed the ARROW-13174 branch 5 times, most recently from 2033695 to 8ce4ab4 Compare July 13, 2021 14:22

rok force-pushed the ARROW-13174 branch from 8ce4ab4 to 249133f Compare July 13, 2021 18:33

rok force-pushed the ARROW-13174 branch from 64442e1 to 08b4bd0 Compare August 17, 2021 12:43

rok force-pushed the ARROW-13174 branch from 08b4bd0 to bc91178 Compare August 17, 2021 14:50

rok commented Aug 17, 2021

View reviewed changes

rok and others added 8 commits August 18, 2021 15:55

Add strftime kernel.

0e8f2fa

Review feedback.

1679aa6

Apply suggestions from code review

3ed11c6

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

Adding locale option.

d44625b

Removing R logic.

00d685d

Changing locale instantiation.

2c9970a

Review feedback.

5ca3f22

Refactor MakeTemporal.

91600d9

pitrou force-pushed the ARROW-13174 branch 2 times, most recently from ff49e17 to 4b1d68a Compare August 18, 2021 14:00

pitrou approved these changes Aug 18, 2021

View reviewed changes

Various changes and improvements

989d2dd

pitrou force-pushed the ARROW-13174 branch from 4b1d68a to 989d2dd Compare August 18, 2021 14:08

Fix std::locale construction

2ab5b5c

pitrou closed this in 2ef0edc Aug 18, 2021

jorisvandenbossche mentioned this pull request Aug 25, 2021

ARROW-13684: [C++][Compute] Strftime kernel follow-up #10998

Closed

This was referenced Aug 25, 2021

[C++][Compute] Add strftime kernel #28874

Closed

[C++][Compute] Strftime kernel follow-up #29320

Closed

Conversation

rok commented Jul 2, 2021

Uh oh!

github-actions bot commented Jul 2, 2021

Uh oh!

rok commented Jul 9, 2021

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rok Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rok commented Jul 13, 2021

Uh oh!

pitrou commented Aug 17, 2021

Uh oh!

rok commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Aug 18, 2021

Uh oh!

rok commented Aug 18, 2021

Uh oh!

jorisvandenbossche commented Aug 19, 2021

Uh oh!

rok commented Aug 20, 2021

Uh oh!

rok commented Aug 20, 2021

Uh oh!

jorisvandenbossche commented Aug 23, 2021

Uh oh!

rok commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rok Jul 13, 2021 •

edited

Loading