ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176

Closed

rok wants to merge 17 commits intoapache:masterfrom

rok:ARROW-11759

Member

rok commented Apr 27, 2021

This is to resolve ARROW-11759.

github-actions bot commented Apr 27, 2021

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

github-actions bot added the Component: C++ label

Member Author

rok commented Apr 27, 2021

Missing features at moment:

isocalendar
millisecond
microsecond
nanosecond

Different input / output types should also be handled.
Does this need to be timezone aware?

Member

jorisvandenbossche commented Apr 28, 2021

Does this need to be timezone aware?

Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?

rok force-pushed the ARROW-11759 branch 4 times, most recently from 2c4968d to 2765cef Compare

May 6, 2021 16:20

Member Author

rok commented May 6, 2021

It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.

Open questions I have at the moment:

Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
What would be a good array type for isodate?

rok marked this pull request as ready for review

May 6, 2021 17:22

Member Author

rok commented May 8, 2021

I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).

pitrou requested changes

View reviewed changes

Member

pitrou left a comment

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

Member

jorisvandenbossche commented May 10, 2021

General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)

rok force-pushed the ARROW-11759 branch from 2765cef to ca2a626 Compare

May 12, 2021 21:22

Member Author

rok commented May 12, 2021

General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)

Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.

rok force-pushed the ARROW-11759 branch 8 times, most recently from f8c6665 to 23f51e7 Compare

May 17, 2021 02:44

jorisvandenbossche reviewed

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

rok force-pushed the ARROW-11759 branch from 23f51e7 to 8974227 Compare

May 17, 2021 12:37

jorisvandenbossche reviewed

View reviewed changes

cpp/src/arrow/compute/api_scalar.h Outdated Show resolved Hide resolved

rok force-pushed the ARROW-11759 branch 3 times, most recently from 7bcc910 to 69c5760 Compare

May 20, 2021 18:26

rok force-pushed the ARROW-11759 branch from 69c5760 to dbc1ec8 Compare

May 25, 2021 11:39

rok and others added 9 commits

June 8, 2021 15:01


          Adding iso_calendar and iso_year kernels.

1326a36


          Fixed kernel signature issue.

5679e63


          Change to ISOCalendar kernel.

251545c


          Implementing review feedback.

76a417d


          Implementing review feedback. #2

9591f91


          Implementing review feedback. #3

dfbfe36


          Update cpp/src/arrow/compute/api_scalar.h

2dc08e9

Co-authored-by: Joris Van den Bossche <[email protected]>


          Apply suggestions from code review

1a16757

Co-authored-by: Joris Van den Bossche <[email protected]>


          Implementing review feedback. #4

8dfe6c2

rok force-pushed the ARROW-11759 branch from d01c351 to 8dfe6c2 Compare

June 8, 2021 13:07

jorisvandenbossche reviewed

View reviewed changes

Member

jorisvandenbossche left a comment

Thanks for the updates! Did a final pass through the docs.

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated Show resolved Hide resolved

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

+                  {"values"}};
+              const FunctionDoc millisecond_doc{
+                  "Extract millisecond values",

Member

jorisvandenbossche Jun 9, 2021

Can you add here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)

Member Author

rok Jun 9, 2021

Done!

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

+                  {"values"}};
+              const FunctionDoc subsecond_doc{
+                  "Extract subsecond values",

Member

jorisvandenbossche Jun 9, 2021

Here also best to add the clarification of what subsecond is from the Subsecond docstring

Member Author

rok Jun 9, 2021

Done!

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

+                }
+                return {static_cast<int64_t>(static_cast<int32_t>(y)),
+                        static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
+                        static_cast<int64_t>(weekday(ymd).iso_encoding())};

Member

jorisvandenbossche Jun 9, 2021

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?

Member Author

rok Jun 9, 2021

Added to docs and prepended.

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

rok force-pushed the ARROW-11759 branch 2 times, most recently from 4dc50cb to ee53452 Compare

June 9, 2021 12:03

Member Author

rok commented Jun 9, 2021

Thanks for the comments and suggestions @jorisvandenbossche! :)

pitrou reviewed

View reviewed changes

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


          Apply suggestions from code review

6625c14

Co-authored-by: Joris Van den Bossche <[email protected]>

rok force-pushed the ARROW-11759 branch from ee53452 to 6625c14 Compare

June 9, 2021 12:52

pitrou approved these changes

View reviewed changes

Member

pitrou commented Jun 9, 2021

Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.

jorisvandenbossche reviewed

View reviewed changes

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


          Update docs/source/cpp/compute.rst

16ef106

Co-authored-by: Joris Van den Bossche <[email protected]>

Member Author

rok commented Jun 9, 2021

Just noticed I missed this:

@jorisvandenbossche
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?

Do you mind if I move it to ARROW-12980?

Member Author

rok commented Jun 9, 2021

Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?
Like ` "1970-01-01T00:00:59.1234567890123"?

pitrou closed this in

00cc41f

rok deleted the ARROW-11759 branch

June 9, 2021 19:04

Member

jorisvandenbossche commented Jun 10, 2021

Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)

what do you mean by "doesn't fit into the nanosecond range"?

I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).

Member Author

rok commented Jun 10, 2021

Thanks @jorisvandenbossche @pitrou for the help and feedback! I'm very glad this is merged! :)

rok mentioned this pull request

ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware #10457

Closed

asfimport mentioned this pull request

[C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type #27613

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels