ARROW-13548: [C++] Implement temporal difference kernels by lidavidm · Pull Request #10960 · apache/arrow

lidavidm · 2021-08-18T16:09:53Z

Implements kernels to get the number of {years, months, days and millis, days, hours, minutes, seconds, millis, micros, nanos} between two timestamps. Everything returns either Int64 or DayTimeInterval.

lidavidm · 2021-08-18T16:10:28Z

I think Python doesn't have bindings for the interval types - that should be fixed in a separate issue I would think.

github-actions · 2021-08-18T16:25:10Z

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

ianmcook · 2021-08-23T18:00:04Z

Would it be straightforward to add a week variant of this that takes DayOfWeekOptions?

lidavidm · 2021-08-23T18:07:56Z

Would it be straightforward to add a week variant of this that takes DayOfWeekOptions?

It should be doable. I'll update the PR.

nealrichardson · 2021-09-15T14:55:48Z

@rok can you take a look here?

lidavidm · 2021-09-15T14:56:20Z

I'll *rebase this in a little while

rok

Thanks @lidavidm !
Couple of comments :)

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

rok · 2021-09-15T20:42:04Z

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

I don't have a better suggestion, but boundaries sounds very implementation-like.

How about this?

const FunctionDoc years_between_doc{ "Compute the number of years between two timestamps", ("Returns the number of year boundaries crossed from the first timestamp to the\n" "second. That is, the difference is calculated as if the timestamps were\n" "truncated to the year.\n" "Null values emit null."), {"start", "end"}};

Better. How about using 'complete years passed/elapsed/..' instead of 'year boundaries crossed'?

I suppose the reason why I'm using 'boundaries' is because the difference between 1998-12-01 and 1999-01-1 is '1 year' here even though a complete year hasn't passed.

Oh, I thought the entire year needs to pass. Right, boundaries are better yeah.

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

rok · 2021-09-15T21:32:33Z

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

Perhaps you could find the first week starting weekday after the starting date and then calculate the difference // 7?

lidavidm · 2021-09-16T18:22:16Z

Changes:

Simplified the week kernel (thanks for the suggestion)
Added docs
Added kernels to get differences as month interval and as month-day-nano interval
Note the interval types are not bound in Python, see ARROW-14018.

rok · 2021-09-16T19:31:27Z

I'll take another look tomorrow. But this already looks pretty good.

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

rok · 2021-09-16T23:08:05Z

docs/source/cpp/compute.rst

Sometimes you use _interval_ and sometimes you don't. I realize it's due to the type returned but it still bothers my eyes :). Probably best keep it as is.

Yeah, I wasn't sure how to disambiguate these. Note that month_interval_between and months_between are basically redundant, except MonthIntervalType is int32_t, not int64_t.

Well, is it useful to have a int64_t return?

Not particularly, except for consistency; that said for Python, we don't have the interval types bound yet, so there'll be a period where this kernel is unavailable if we remove this version. I think overall it's not worth keeping it though so I'll remove it.

rok · 2021-09-17T16:26:40Z

LGTM @lidavidm!
Perhaps it would be good to ask for another review - I might be missing something.

pitrou · 2021-09-20T14:16:40Z

cpp/src/arrow/util/formatting_util_test.cc

Is it a recommended format for months? Should we use "M" instead?

I was following the existing formatting of the array printers, but M probably makes more sense/I can align these with strftime.

Actually, I suppose that doesn't make any sense. M seems preferable to m though so I'll do that.

pitrou · 2021-09-20T14:17:56Z

docs/source/cpp/compute.rst

Well, is it useful to have a int64_t return?

pitrou · 2021-09-20T14:19:11Z

docs/source/cpp/compute.rst

If the "notes" column is empty, then can omit it.

pitrou · 2021-09-20T14:20:52Z

docs/source/cpp/compute.rst

"has a defined timezone" is less weird than "is zoned".

pitrou · 2021-09-20T14:22:17Z

docs/source/cpp/compute.rst

I would say "because the local year is the same even though the UTC years would be different".

pitrou · 2021-09-20T14:26:41Z

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

Is MakeArrayFromScalar necessary or is the kernel able to broadcast a scalar input?

pitrou · 2021-09-20T14:35:52Z

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

At some point, we may instead define a Validate() method on FunctionOptions.

pitrou · 2021-09-20T14:36:34Z

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

Nit, but "Extract" refers to component extraction, which this isn't doing?

pitrou · 2021-09-20T14:44:42Z

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

How about: Return the number of year boundaries crossed from `start` to `end`.

Co-authored-by: Rok Mihevc <[email protected]>

lidavidm · 2021-09-27T19:15:29Z

I've rebased this.

rok

Looks great @lidavidm !

Do we want to support this for time/date/32/64 at some point?

rok · 2021-09-27T20:07:36Z

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

+  static int64_t GetQuarters(const year_month_day& ymd) {
+    return static_cast<int64_t>(static_cast<int32_t>(ymd.year())) * 4 +
+           (static_cast<uint32_t>(ymd.month()) - 1) / 3;
+  }


You could use this function for the quarter extractor and delete this logic there.

I mean this:

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

Lines 405 to 410 in 7124872

template <typename T, typename Arg0>

T Call(KernelContext*, Arg0 arg, Status*) const {

const auto ymd =

year_month_day(floor<days>(localizer_.template ConvertTimePoint<Duration>(arg)));

return static_cast<T>((static_cast<const uint32_t>(ymd.month()) - 1) / 3 + 1);

}

lidavidm · 2021-09-27T20:31:19Z

Looks great @lidavidm !

Do we want to support this for time/date/32/64 at some point?

Thanks for the review. Yeah, let me add support for the other temporal types - I suppose all that's merged by now.

…imes

lidavidm · 2021-09-28T18:01:10Z

I've added the support for date/time types (years, months, etc. don't accept time types).

rok

Looks good!

docs/source/cpp/compute.rst

rok · 2021-09-28T22:08:01Z

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

+          &default_day_of_week_options, DayOfWeekState::Init);
+  DCHECK_OK(registry->AddFunction(std::move(weeks_between)));
+
+  auto day_time_between =


Nit: day_time_interval_between would be more consistent. Same for month_day_nano_interval_between. I don't feel strongly about it, just noticing :).

lidavidm · 2021-09-28T22:20:29Z

Thanks, fixed those issues.

rok · 2021-09-28T23:20:46Z

I think this is ready to go.

Implements kernels to get the number of {years, months, days and millis, days, hours, minutes, seconds, millis, micros, nanos} between two timestamps. Everything returns either Int64 or DayTimeInterval. Closes apache#10960 from lidavidm/arrow-13548 Authored-by: David Li <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added the Component: C++ label Aug 18, 2021

lidavidm force-pushed the arrow-13548 branch from 20e790b to 371fa28 Compare August 18, 2021 18:06

lidavidm force-pushed the arrow-13548 branch from 371fa28 to 08d053b Compare August 23, 2021 21:32

lidavidm force-pushed the arrow-13548 branch 2 times, most recently from 5bcb371 to f4ce70e Compare September 9, 2021 15:34

lidavidm force-pushed the arrow-13548 branch 2 times, most recently from 55dcad6 to 342e6d8 Compare September 15, 2021 15:50

rok reviewed Sep 15, 2021

View reviewed changes

lidavidm force-pushed the arrow-13548 branch from 4d17d71 to 6807df5 Compare September 16, 2021 18:05

rok reviewed Sep 16, 2021

View reviewed changes

lidavidm force-pushed the arrow-13548 branch from 66b79bc to 7dbd1ee Compare September 17, 2021 15:33

lidavidm requested a review from pitrou September 20, 2021 13:27

pitrou reviewed Sep 20, 2021

View reviewed changes

lidavidm force-pushed the arrow-13548 branch 2 times, most recently from 20742d5 to 5a5421d Compare September 27, 2021 15:44

ARROW-13548: [C++] Implement temporal difference kernels

7124872

Co-authored-by: Rok Mihevc <[email protected]>

lidavidm force-pushed the arrow-13548 branch from 5a5421d to 7124872 Compare September 27, 2021 16:52

rok reviewed Sep 27, 2021

View reviewed changes

ARROW-13548: [C++] Implement temporal difference kernels for dates, t…

7ed878b

…imes

ARROW-13548: [C++] Update docs

9d0a83a

rok approved these changes Sep 28, 2021

View reviewed changes

ARROW-13548: [C++] Fix suggestions

e24f393

pitrou approved these changes Sep 29, 2021

View reviewed changes

pitrou closed this in 661c7d7 Sep 29, 2021

lidavidm deleted the arrow-13548 branch September 29, 2021 11:57

asfimport mentioned this pull request Sep 30, 2021

[C++] Implement datediff kernel #29199

Closed

	template <typename T, typename Arg0>
	T Call(KernelContext, Arg0 arg, Status) const {
	const auto ymd =
	year_month_day(floor<days>(localizer_.template ConvertTimePoint<Duration>(arg)));
	return static_cast<T>((static_cast<const uint32_t>(ymd.month()) - 1) / 3 + 1);
	}

Conversation

lidavidm commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Aug 18, 2021

Uh oh!

github-actions bot commented Aug 18, 2021

Uh oh!

ianmcook commented Aug 23, 2021

Uh oh!

lidavidm commented Aug 23, 2021

Uh oh!

nealrichardson commented Sep 15, 2021

Uh oh!

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

Uh oh!

Uh oh!

rok left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Sep 16, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

rok commented Sep 17, 2021

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!

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

lidavidm commented Aug 18, 2021 •

edited

Loading

lidavidm commented Sep 15, 2021 •

edited

Loading

rok commented Sep 16, 2021 •

edited

Loading

rok Sep 27, 2021 •

edited

Loading