ARROW-14098: [C++] subtract(time, time) -> duration kernel

[rok]

This is to resolve ARROW-14098.

[github-actions]

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

[github-actions]

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

[edponce]

LGTM!

[rok]

Thanks for the review @edponce! I'm doubting myself a bit here - the Jira is asking for interval return type while this returns time at the moment.
@cpcloud are we looking to return interval (DayTimeIntervalType, MonthIntervalType, MonthDayNanoIntervalType) or time (Time32Type, Time64Type) here?
I suppose both would be useful.

[edponce]

Oops! Good that you noticed.

[rok]

Although returning time is kind of semantically wrong. Duration is already covered so intervals it is.

[jorisvandenbossche]

Although returning time is kind of semantically wrong. Duration is already covered so intervals it is.

Agreed that returning "time" is wrong. How is duration already covered?

[rok]

Agreed that returning "time" is wrong. How is duration already covered?

Ah, I was thinking of subtract(timestamp, timestamp) -> duration. subtract(time, time) -> duration is not covered yet.

[jorisvandenbossche]

So if "subtract" for timestamp results in duration type, I think subtract(time, time) should also give duration? (and not interval)

And it seems that for subtracting time values to get an interval, we actually already have this covered with the "<interval type>_between" kernels:

In [14]: reg = pc.function_registry()
In [15]: reg.get_function("month_day_nano_interval_between").kernels
Out[15]: [ScalarKernel<(any[date32[day]], any[date32[day]]) -> month_day_nano_interval>,
ScalarKernel<(any[date64[ms]], any[date64[ms]]) -> month_day_nano_interval>,
ScalarKernel<(any[time32[s]], any[time32[s]]) -> month_day_nano_interval>,
ScalarKernel<(any[time32[ms]], any[time32[ms]]) -> month_day_nano_interval>,
ScalarKernel<(any[time64[us]], any[time64[us]]) -> month_day_nano_interval>,
ScalarKernel<(any[time64[ns]], any[time64[ns]]) -> month_day_nano_interval>,
ScalarKernel<(any[timestamp(s)], any[timestamp(s)]) -> month_day_nano_interval>,
ScalarKernel<(any[timestamp(ms)], any[timestamp(ms)]) -> month_day_nano_interval>,
ScalarKernel<(any[timestamp(us)], any[timestamp(us)]) -> month_day_nano_interval>,
ScalarKernel<(any[timestamp(ns)], any[timestamp(ns)]) -> month_day_nano_interval>]

[rok]

Switched to duration output.

[pitrou]

Thank you. Can you also update docs/source/cpp/compute.rst for the newly supported types (or do you do that in another related PR)?

[rok]

As pointed out by Joris and David we should use DispatchBest to promote type when subtract(time32, time64).

[rok]

As pointed out by Joris and David we should use DispatchBest to promote type when subtract(time32, time64).

Added type promotion. Reviews welcome!

[jorisvandenbossche]

Didn't yet look at the code, but if we having type casting for temporal types during execution, best to add some tests where casting would be unsafe (eg a value in seconds resolution that would not fit in nanosecond resolution)

[pitrou]

@rok Are you done with this? I see you're still pushing changes.

[rok]

@pitrou It's done, I just noticed the kernel output type was off. Please proceed :).

[pitrou]

Hmm, can you look at the test failures?

[rok]

@pitrou I think this is ready for review.

[pitrou]

Thanks for the update @rok. Here are some more comments.

[pitrou]

I'm not sure I understand what it means to cast from a point in time to a duration. Does it make sense?

[rok]

Turns out we can just subtract times so this is not really needed anymore.

[pitrou]

If these are basically the same checks, why not factor them out?

[rok]

Done.

[pitrou]

I don't see any tests for this, do we need to add some?

[rok]

Because subtraction can be done directly on times I removed this.

[rok]

@pitrou Thanks for review! I removed most of the things :). Could you do another pass?

[pitrou]

+1, thanks for the updates @rok

[ursabot]

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