ARROW-14822: [C++] Implement floor/ceil/round for temporal objects #11818

rok · 2021-11-30T20:46:45Z

This is to resolve ARROW-14822.

github-actions · 2021-11-30T20:47:05Z

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

github-actions · 2021-11-30T20:47:06Z

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

cpp/src/arrow/compute/api_scalar.h

cpp/src/arrow/compute/api_scalar.cc

rok · 2021-12-14T23:46:18Z

This is getting review ready, couple of issues remaining:

RoundTemporal, CeilTemporal and FloorTemporal could maybe be templated from a single struct
Nonexistent in RoundTemporalOptions might be unnecessary as we assume timestamps are in UTC so they always exist. Probably holds for Ambiguous too.
Test data needs some work, especially pre-1970 and edge-cases.

rok · 2021-12-14T23:47:42Z

@lidavidm @jorisvandenbossche could you take a quick glance to see if something needs to be done fundamentally different here? If not I hope to have this review ready tomorrow.

lidavidm · 2021-12-14T23:50:44Z

Nonexistent in RoundTemporalOptions might be unnecessary as we assume timestamps are in UTC so they always exist. Probably holds for Ambiguous too.

Would we want to eventually support rounding in a timezone? (That can be deferred since I can see that getting tricky.) e.g. rounding to the hour may differ in a timezone that is xx:30 offset from UTC.

rok · 2021-12-14T23:53:50Z

Nonexistent in RoundTemporalOptions might be unnecessary as we assume timestamps are in UTC so they always exist. Probably holds for Ambiguous too.

Would we want to eventually support rounding in a timezone? (That can be deferred since I can see that getting tricky.) e.g. rounding to the hour may differ in a timezone that is xx:30 offset from UTC.

I've been thinking about that exact situation (xx:30) and I'm not sure yet how to tackle it but I'd like to in this ticket.

lidavidm · 2021-12-14T23:56:18Z

I only took a quick scan but I don't see anything fundamentally wrong.

It gets confusing with all the chrono types but we should stick to UpperCamelCase for Arrow functions/methods
Do we want QUARTER in addition to SEASON?

rok · 2021-12-15T00:04:40Z

Do we want QUARTER in addition to SEASON?

I had it originally but it felt redundant as it's just RoundTemporal(arg, RoundTemporalOptions(unit=month, multiple=3)). I'll add it back as it will probably get requested.

lidavidm · 2021-12-15T00:05:10Z

Ah, that is true - we could just document it.

rok · 2021-12-21T12:10:09Z

@lidavidm - I think this is ready for first round of review.
I've decided to remove Nonexistent and Ambiguous.
I'm working out some issues with rounding multiple seasons but other than that I think it's good to review. I'll also try adding origin to set the "zero time" in one of the future commits.

rok · 2021-12-21T12:53:56Z

@pitrou any thoughts on this?

lidavidm

Overall looks good to me, taking into account the TODOs.

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

cpp/src/arrow/compute/api_scalar.h

python/pyarrow/_compute.pyx

python/pyarrow/tests/test_compute.py

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

cpp/src/arrow/compute/api_scalar.h

jorisvandenbossche · 2021-12-22T07:04:05Z

I've decided to remove Nonexistent and Ambiguous.

(didn't yet look at the code) Did you then also remove support rounding timestamps with a timezone? Or how does rounding tz-aware timestamps work?

It's also not really clear to me what origin would do. Can you explain with an example what it would do? (I also don't see it in lubridate or pandas?)

rok · 2021-12-22T11:23:57Z

I've decided to remove Nonexistent and Ambiguous.

(didn't yet look at the code) Did you then also remove support rounding timestamps with a timezone? Or how does rounding tz-aware timestamps work?

At the moment some of the code (ns, us, ms, s, m, h, d, w) is UTC based even when timezones are present. UTC always converts to a local time so nonexistent is not an issue while ambiguous could be.
Month, quarter, year are currently rounded in local time and integer multiples of those "units" are less problematic because beginnings of months seem to always exist. Perhaps this needs more study.

It's also not really clear to me what origin would do. Can you explain with an example what it would do? (I also don't see it in lubridate or pandas?)

Origin is meant like the origin in Euclidean space. Let's say you want to floor today's date to 100year:

floor("2021-12-22", unit="year", multiple=100, origin="1970-01-01") == "1970-01-01"
floor("2021-12-22", unit="year", multiple=100, origin="0-01-01") == "2000-01-01"

R's clock has this concept.
It's also a "replacement" for week_starts_monday.
e.g.: 1970-01-01 was a Thursday so floor(t, unit="week", multiple=1, origin="1970-01-05") is equivalent to floor(t, unit="week", multiple=1, week_starts_monday=True)

rok · 2021-12-23T00:36:01Z

I think tests are now mostly in order. Remaining work:

origin
timezone handling (nonexistent, ambiguous)
shorthand

Am I forgetting something?

Which of these could be pushed out of this scope? I'm thinking shorthand and perhaps perhaps timezone handling.
@lidavidm @jorisvandenbossche

rok · 2022-01-03T01:38:30Z

@lidavidm @jorisvandenbossche this now rounds time since epoch in local time (see here). It works ok outside of DST and discussible within. It's a different approach to what e.g. Pandas does.
I'm going to look into a better approach for this.

python/pyarrow/tests/test_compute.py

lidavidm · 2022-01-04T13:42:24Z

python/pyarrow/tests/test_compute.py

+    timestamps = [
+        # "1899-04-18 01:57:09.190202880",
+        # "1899-09-12 07:03:30.080325120",
+        # "1904-06-21 20:55:36.493869056",


Are these still meant to be commented out?

There are some odd changes in timezone offsets that are causing problems. I'll try changing ceil to match behaviour.

This is the kind of error I'm seeing for these three timestamps:

actual:

Timestamp('1899-04-19 00:31:50+0553', tz='Asia/Kolkata') Timestamp('1899-09-13 00:31:50+0553', tz='Asia/Kolkata') Timestamp('1904-06-22 23:59:50+0521', tz='Asia/Kolkata')

expected:

Timestamp('1899-04-19 00:00:00+0553', tz='Asia/Kolkata'), Timestamp('1899-09-13 00:00:00+0553', tz='Asia/Kolkata'), Timestamp('1904-06-23 00:00:00+0521', tz='Asia/Kolkata')

Calcutta time was one of the two time zones established in British India in 1884. It was established during the International Meridian Conference held at Washington, D.C. in the United States. It was decided that India had two time zones: Calcutta (now Kolkata) would use the 90th meridian east and Bombay (now Mumbai) the 75th meridian east. It was determined as 5 hours, 53 minutes and 20 seconds ahead of Greenwich Mean Time(UTC+5:53:20).

https://en.wikipedia.org/wiki/Calcutta_Time

Oh boy. So, if I'm reading this right, it's rounding using the "modern" TZ definition instead of the "contemporary" one? Maybe we should just not test this case then, since this is down to whatever the timezone database for a system contains…? Or am I misunderstanding.

I think it's that but I'm not 100% and I really want to be :))

Sorry I mislabeled that (switched actual and expected, fixed now). It appears my rounding in local time is not done correctly.

I'm back to thinking Pandas only uses "modern" TZs while date.h uses a historical database.
I've added a test for this in c++ and removed it from Python.

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

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

lidavidm · 2022-01-04T19:48:21Z

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

nit: for context, can we note that this test is here to test the a case where date (and hence our kernels) uses historical TZ info while Pandas (and possibly other libraries) do not?

Otherwise I fear confusion when someone in the future looks and wonders why Kolkata was an especially important time zone to test :)

Yeah, that makes sense.

Comment added.

It seems reality is more nuanced than Wikipedia states.

lidavidm · 2022-01-04T19:50:04Z

Oh, and we need to add this to the python api.rst as well.

rok · 2022-01-04T22:00:05Z

Thanks @lidavidm & @jorisvandenbossche ! :)

Follow-up Jiras:

shorthand - ARROW-15250
additional timezone handling options (nonexistent, ambiguous) - ARROW-15251

ursabot · 2022-01-05T04:46:13Z

Benchmark runs are scheduled for baseline = acce03b and contender = edab145. edab145 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
[Finished ⬇️0.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.09% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the Component: C++ label Nov 30, 2021

lidavidm reviewed Nov 30, 2021

View reviewed changes

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

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

rok force-pushed the ARROW-14822 branch 4 times, most recently from c986aeb to 5b1fe4b Compare December 6, 2021 10:50

github-actions bot added the Component: Python label Dec 6, 2021

rok force-pushed the ARROW-14822 branch 2 times, most recently from 0988d48 to 0814df6 Compare December 14, 2021 23:37

rok force-pushed the ARROW-14822 branch from 0814df6 to c3816c0 Compare December 14, 2021 23:50

rok force-pushed the ARROW-14822 branch from c3816c0 to dab3372 Compare December 15, 2021 00:10

rok marked this pull request as ready for review December 21, 2021 12:04

lidavidm reviewed Dec 21, 2021

View reviewed changes

rok force-pushed the ARROW-14822 branch from c28f7fa to 1582608 Compare December 22, 2021 01:55

rok force-pushed the ARROW-14822 branch 2 times, most recently from b241c3c to e1e55d0 Compare December 23, 2021 11:11

rok added 3 commits December 31, 2021 16:19

Review feedback.

142c2eb

Further work on unit multiples.

6d5e171

Timezone handling.

04c7c93

rok force-pushed the ARROW-14822 branch 3 times, most recently from 4429c20 to d84408d Compare January 1, 2022 17:43

Refactoring tz logic.

08ce53f

rok force-pushed the ARROW-14822 branch 2 times, most recently from ed1f711 to b93f075 Compare January 3, 2022 01:25

rok force-pushed the ARROW-14822 branch from b93f075 to c68e36e Compare January 4, 2022 02:04

Remove origin.

8d844d1

rok force-pushed the ARROW-14822 branch from c68e36e to 8d844d1 Compare January 4, 2022 02:58

lidavidm reviewed Jan 4, 2022

View reviewed changes

rok force-pushed the ARROW-14822 branch 3 times, most recently from 4be4c66 to e2b6d9f Compare January 4, 2022 19:35

lidavidm approved these changes Jan 4, 2022

View reviewed changes

rok force-pushed the ARROW-14822 branch from e2b6d9f to ef86d30 Compare January 4, 2022 19:49

rok force-pushed the ARROW-14822 branch 2 times, most recently from c7cf795 to e8f9a62 Compare January 4, 2022 20:10

Changes to ceil.

89141f5

rok force-pushed the ARROW-14822 branch from e8f9a62 to 89141f5 Compare January 4, 2022 20:17

lidavidm approved these changes Jan 4, 2022

View reviewed changes

lidavidm closed this in edab145 Jan 4, 2022

rok mentioned this pull request Jun 8, 2022

ARROW-15251: [C++] Temporal floor/ceil/round handle ambiguous/nonexistent local time #12528

Closed

asfimport mentioned this pull request Jan 5, 2022

[C++] Implement floor/ceil/round for temporal objects #30354

Closed

ARROW-14822: [C++] Implement floor/ceil/round for temporal objects #11818

ARROW-14822: [C++] Implement floor/ceil/round for temporal objects #11818

Uh oh!

Conversation

rok commented Nov 30, 2021

Uh oh!

github-actions bot commented Nov 30, 2021

Uh oh!

github-actions bot commented Nov 30, 2021

Uh oh!

Uh oh!

Uh oh!

rok commented Dec 14, 2021

Uh oh!

rok commented Dec 14, 2021

Uh oh!

lidavidm commented Dec 14, 2021

Uh oh!

rok commented Dec 14, 2021

Uh oh!

lidavidm commented Dec 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

lidavidm commented Dec 15, 2021

Uh oh!

rok commented Dec 21, 2021

Uh oh!

rok commented Dec 21, 2021

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorisvandenbossche commented Dec 22, 2021

Uh oh!

rok commented Dec 22, 2021

Uh oh!

rok commented Dec 23, 2021

Uh oh!

rok commented Jan 3, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rok Jan 4, 2022 • 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!

rok Jan 4, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rok Jan 4, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

rok commented Dec 15, 2021 •

edited

Loading

rok Jan 4, 2022 •

edited

Loading

rok Jan 4, 2022 •

edited

Loading

rok Jan 4, 2022 •

edited

Loading

rok commented Jan 4, 2022 •

edited

Loading

ursabot commented Jan 5, 2022 •

edited

Loading