ARROW-14821: [C++] Add ceil_is_strictly_greater and calendar_based_origin temporal round options (to mimic lubridate's date rounding) by rok · Pull Request #12657 · apache/arrow

rok · 2022-03-17T17:39:15Z

This implements RoundTemporalOptions.ceil_is_strictly_greater and RoundTemporalOptions.calendar_based_origin parameters in C++ to enable temporal rounding in R (ARROW-14821).

github-actions · 2022-03-17T17:39:34Z

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

github-actions · 2022-03-17T17:39:36Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

rok · 2022-03-17T17:41:21Z

@djnavarro this is almost ready for review.

rok · 2022-03-30T19:46:15Z

@djnavarro do you think this will cover requirements on R side?
I've tried to use R to create the test data where possible and do sanity checks but I'm slowly developing test blindness :)

@pitrou @jorisvandenbossche could you please review the changes here? The R style rounding demanded some more logic and maybe something I'm missing something that can be simplified.
Also: perhaps we can find better variable names to replace change_on_boundary and calendar_based_origin?

rok · 2022-04-06T12:22:25Z

ping :)

pitrou

Congrats for working on this @rok. Here are some comments.

pitrou · 2022-04-13T10:24:12Z

Since this is only for ceil, perhaps rename it strict_ceil or something similar? @jorisvandenbossche what do you think?

Switched to strict_ceil. Would still be interesting to hear from @jorisvandenbossche

@jorisvandenbossche ping :)

Do we know if "strict ceil" is a typical term for this behaviour?
(it could also be "ceil_on_boundary" to make it more explicit about "ceil")

Switched to ceil_on_boundary. @pitrou do you think that'll work?

rok · 2022-04-21T13:22:50Z

@pitrou Could you please do another pass here?

pitrou

I don't have much to add, see below.

rok · 2022-04-21T14:49:32Z

@pitrou Added suggested comments.

pitrou

Thanks! I'll let @jorisvandenbossche do the final review.

rok · 2022-04-22T00:24:24Z

CI issues seem unrelated.

rok · 2022-04-22T14:53:50Z

@jorisvandenbossche this would unblock #12154 which would be great to have for the release :).

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

rok · 2022-05-30T19:00:28Z

There was the issue about calendar_based_origin=True potentially breaking sortedness (as discussed in #12528).
Are we fine with this caveat? Maybe yes, since it is also what lubridate does (but mention it in the docs?). Or would it be possible to guard the user from that ever happening?

I think we're ok with calendar_based_origin=True breaking sortedness as it would be an explicitly chosen way of rounding for users outside of lubridate. I'll add a note about this to docs.
We could also introduce perserve_sortedness flag in #12528 and handle calendar_based_origin=True case.

For example, if you round by "x" minutes, the origin unit is an hour, and in that case we could error if one hour is not dividable by "x"? Although for hours and below that can work, but I suppose for days, months, years that is not possible?

Lubridate actually limits to time multiples smaller or equal than the greater time unit - e.g. "61min" is not a valid rounding, but "60min" is. Your proposal makes things safer but we might want to "deregulate" it later. Leaving things as is gives more options but we might need to limit them later. Dunno, I don't have an opinion on this.

rok · 2022-05-30T20:02:37Z

@jorisvandenbossche added an example to doc to present the issue with calendar_based_origin=True.

rok · 2022-05-31T11:56:23Z

@pitrou do you think this is ready to merge?

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou · 2022-05-31T12:13:15Z

TBH, I'm starting to think that calendar-based rounding is sufficiently different that we might define separate functions for it (e.g. "ceil_calendar" as opposed to "ceil_temporal"). We might also not bother refactoring this again :-)

rok · 2022-05-31T12:16:01Z

Thanks for the review, added the changes.

TBH, I'm starting to think that calendar-based rounding is sufficiently different that we might define separate functions for it (e.g. "ceil_calendar" as opposed to "ceil_temporal"). We might also not bother refactoring this again :-)

I had that thought yesterday too. Now that tests are worked out it wouldn't be too much work I feel. Perhaps after #12528 is in :).

rok · 2022-06-01T12:36:19Z

@pitrou or are you saying we should refactor now? :)

pitrou · 2022-06-01T13:23:15Z

@rok As you prefer. We can merge this as is if you want.

rok · 2022-06-01T13:24:49Z

@pitrou i would prefer merging this and then discuss/do the refactor separately.

pitrou · 2022-06-02T12:55:26Z

Ok, let's do it.

rok · 2022-06-02T17:18:20Z

Thank you for the reviews and feedback @pitrou & @jorisvandenbossche !

github-actionsBot added Component: C++ Component: Python labels Mar 17, 2022

rok force-pushed the ARROW-14821-change_on_boundary branch 4 times, most recently from b25de93 to f65728cCompare March 30, 2022 13:48

rok mentioned this pull request Mar 30, 2022
ARROW-15251: [C++] Temporal floor/ceil/round handle ambiguous/nonexistent local time #12528
Open

rok force-pushed the ARROW-14821-change_on_boundary branch 2 times, most recently from ba00ddb to 92ad204Compare March 30, 2022 19:27

rok marked this pull request as ready for review March 30, 2022 19:29

rok mentioned this pull request Mar 31, 2022
ARROW-14821: [R] Implement bindings for lubridate's floor_date, ceiling_date, and round_date #12154
Merged

rok changed the title ~~ARROW-14821: [R] Implement bindings for lubridate's floor_date, ceiling_date, and round_date~~ARROW-14821: [C++][R] Implement bindings for lubridate's floor_date, ceiling_date, and round_dateApr 1, 2022

pitrou reviewed Apr 13, 2022
View reviewed changes

rok force-pushed the ARROW-14821-change_on_boundary branch 2 times, most recently from 657b3d7 to 3ae71e7Compare April 21, 2022 13:19

pitrou approved these changes Apr 21, 2022
View reviewed changes

Comment threadpython/pyarrow/_compute.pyx Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated

rok force-pushed the ARROW-14821-change_on_boundary branch 2 times, most recently from 6a919a8 to d5bc076Compare April 21, 2022 14:46

pitrou approved these changes Apr 21, 2022
View reviewed changes

pitrou requested a review from jorisvandenbossche April 21, 2022 14:51

rok force-pushed the ARROW-14821-change_on_boundary branch from d5bc076 to 18a0fdcCompare April 21, 2022 18:09

rokand others added 9 commits May 30, 2022 16:16

Apply suggestions from code review
9fe7a06
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

ceil_on_boundary -> ceil_is_strictly_greater
e9f36fb

Review feedback.
af2a5e3

Update python/pyarrow/_compute.pyx
c5c7cc0
Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Update python/pyarrow/_compute.pyx
b2ff7a2
Co-authored-by: Antoine Pitrou <pitrou@free.fr>

review feedback
65807b7

multiple_since_greater_unit->calendar_based_origin
167236d

Update python/pyarrow/_compute.pyx
b2f9226
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

review feedback
38696a6

Comments for RoundTemporalOptions.calendar_based_origin
422b022

rok force-pushed the ARROW-14821-change_on_boundary branch from 8c2d6ad to 422b022Compare May 30, 2022 19:58

jorisvandenbossche changed the title ~~ARROW-14821: [C++][R] Implement bindings for lubridate's floor_date, ceiling_date, and round_date~~ARROW-14821: [C++] Add ceil_is_strictly_greater and calendar_based_origin temporal round options (to mimic lubridate's date rounding)May 31, 2022

jorisvandenbossche approved these changes May 31, 2022
View reviewed changes

pitrou reviewed May 31, 2022
View reviewed changes

Comment threadpython/pyarrow/_compute.pyx Outdated

pitrou reviewed May 31, 2022
View reviewed changes

Comment threadpython/pyarrow/_compute.pyx Outdated

Apply suggestions from code review
de82528
Co-authored-by: Antoine Pitrou <pitrou@free.fr>

rok requested a review from pitrou May 31, 2022 13:59

pitrou merged commit fc082c5 into apache:masterJun 2, 2022

rok mentioned this pull request Sep 15, 2022
feat: add temporal functions substrait-io/substrait#272
Merged

asfimport mentioned this pull request Jul 22, 2022
[R] Implement bindings for lubridate's floor_date, ceiling_date, and round_date #30353
Closed

Uh oh!

Conversation

rok commented Mar 17, 2022• edited by jorisvandenbossche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Mar 17, 2022

Uh oh!

github-actionsBot commented Mar 17, 2022

Uh oh!

rok commented Mar 17, 2022

Uh oh!

rok commented Mar 30, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rok commented Apr 6, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrouApr 13, 2022

Choose a reason for hiding this comment

Uh oh!

rokApr 21, 2022

Choose a reason for hiding this comment

Uh oh!

rokMay 5, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbosscheMay 5, 2022

Choose a reason for hiding this comment

Uh oh!

rokMay 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rok commented Apr 21, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rok commented Apr 21, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

rok commented Apr 22, 2022

Uh oh!

rok commented Apr 22, 2022

Uh oh!

rok commented May 30, 2022

Uh oh!

rok commented May 30, 2022

Uh oh!

rok commented May 31, 2022

Uh oh!

Uh oh!

Uh oh!

pitrou commented May 31, 2022

Uh oh!

rok commented May 31, 2022

Uh oh!

rok commented Jun 1, 2022

Uh oh!

pitrou commented Jun 1, 2022

Uh oh!

rok commented Jun 1, 2022

Uh oh!

pitrou commented Jun 2, 2022

Uh oh!

rok commented Mar 17, 2022•
edited by jorisvandenbossche
Loading

rok commented Mar 30, 2022•
edited
Loading