Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rok wants to merge 6 commits into from
Closed

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

rok wants to merge 6 commits into from

Conversation

@rok
Copy link
Member

@rok rok commented Jan 8, 2022

This is to resolve ARROW-14098.

@github-actions
Copy link

github-actions bot commented Jan 8, 2022

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

@github-actions
Copy link

github-actions bot commented Jan 8, 2022

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

edponce
edponce approved these changes Jan 9, 2022
View reviewed changes
Copy link
Contributor

@edponce edponce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rok
Copy link
Member Author

rok commented Jan 10, 2022

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
Copy link
Contributor

edponce commented Jan 10, 2022

Oops! Good that you noticed.

@rok
Copy link
Member Author

rok commented Jan 10, 2022

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

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jan 10, 2022

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
Copy link
Member Author

rok commented Jan 10, 2022

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
Copy link
Member

jorisvandenbossche commented Jan 11, 2022

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
Copy link
Member Author

rok commented Jan 11, 2022

Switched to duration output.

rok
rok commented Jan 11, 2022
View reviewed changes
@jorisvandenbossche jorisvandenbossche changed the title ARROW-14098: [C++] subtract(time, time) -> interval kernel ARROW-14098: [C++] subtract(time, time) -> duration kernel Jan 11, 2022
@rok rok force-pushed the ARROW-14098 branch 2 times, most recently from a5ed967 to 9c2140c Compare January 13, 2022 10:39
pitrou
pitrou requested changes Jan 18, 2022
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

rok commented Jan 18, 2022

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

@rok rok force-pushed the ARROW-14098 branch 2 times, most recently from 0fdfd55 to d5f2ddc Compare January 19, 2022 12:27
@rok
Copy link
Member Author

rok commented Jan 19, 2022

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

Added type promotion. Reviews welcome!

@rok rok requested a review from pitrou January 19, 2022 12:30
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jan 19, 2022

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)

@rok rok force-pushed the ARROW-14098 branch 5 times, most recently from b175d89 to 4dc443a Compare January 27, 2022 10:24
@pitrou
Copy link
Member

pitrou commented Jan 27, 2022

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

@rok
Copy link
Member Author

rok commented Jan 27, 2022

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

@pitrou
Copy link
Member

pitrou commented Jan 27, 2022

Hmm, can you look at the test failures?

@rok rok force-pushed the ARROW-14098 branch 3 times, most recently from 9dde52d to 1829f69 Compare February 1, 2022 11:20
@rok
Copy link
Member Author

rok commented Feb 1, 2022

@pitrou I think this is ready for review.

pitrou
pitrou requested changes Feb 1, 2022
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cpp/src/arrow/compute/kernels/scalar_cast_temporal.cc Outdated
Copy link
Member

@pitrou pitrou Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@rok rok Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Copy link
Member

@pitrou pitrou Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@rok rok Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Copy link
Member

@pitrou pitrou Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@rok rok Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rok rok force-pushed the ARROW-14098 branch 2 times, most recently from 18b0f87 to 3c1de4d Compare February 1, 2022 14:27
@rok
Copy link
Member Author

rok commented Feb 1, 2022

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

@rok rok requested a review from pitrou February 8, 2022 10:32
pitrou
pitrou approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks for the updates @rok

@pitrou pitrou closed this in 75bcece Feb 8, 2022
@ursabot
Copy link

ursabot commented Feb 8, 2022 •
edited
Loading

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

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

Reviewers

@pitrou pitrou pitrou approved these changes

+1 more reviewer

@edponce edponce edponce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rok @edponce @jorisvandenbossche @pitrou @ursabot