ARROW-14093: [C++] subtract(date, date) -> duration kernel by rok · Pull Request #12124 · apache/arrow

rok · 2022-01-11T15:47:22Z

This is to resolve ARROW-14093.

github-actions · 2022-01-11T15:47:45Z

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

github-actions · 2022-01-11T15:47:46Z

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

lidavidm · 2022-01-12T13:40:28Z

The title should be -> duration, not -> interval, right?

rok · 2022-01-12T15:21:49Z

The title should be -> duration, not -> interval, right?

I think so. I suppose same goes for most most ARROW-11090 jiras.

rok · 2022-01-12T21:49:41Z

This now always returns in ms. Do we want to be able to output other units or is it ok to defer to casting kernels if other units are desired?

lidavidm · 2022-01-12T21:53:34Z

I don't see a problem with ms.

lidavidm · 2022-01-27T22:03:17Z

Sorry, I've been a bit backlogged, I'll try to get through these tomorrow (Friday)

rok · 2022-01-27T22:33:53Z

No worries @lidavidm! :)

lidavidm

Thanks, just a couple nits.

lidavidm · 2022-02-01T14:07:42Z

nit, but as used, the units should always match right?

It's used to resolve duration resolution in subtract(timestamp, timestamp) -> duration and the timestamps could be of any resolution. Am I missing something?

Ah, I was thinking, since we only register kernels where the units match, technically this max is redundant, but it doesn't hurt to have it. (Though perhaps it would be safer to DCHECK or return an error if they don't match to avoid accidentally doing arithmetic on incompatible types.)

(I think return types are computed post-implicit-casts, right? So they should match here.)

Ah, yes of course! Fixed. Thanks :).

rok · 2022-02-01T14:53:36Z

Thanks for the review @lidavidm! I've accidentally amended changes to the last commit, sorry.
I've also refactored some other temporal subtraction tests to avoid duplicating subtract and subtract_checked logic.

lidavidm

Thanks, just some nits/suggestions (sorry for the back-and-forth)

lidavidm · 2022-02-01T22:45:33Z

Ah, I was thinking, since we only register kernels where the units match, technically this max is redundant, but it doesn't hurt to have it. (Though perhaps it would be safer to DCHECK or return an error if they don't match to avoid accidentally doing arithmetic on incompatible types.)

lidavidm · 2022-02-01T22:45:51Z

(I think return types are computed post-implicit-casts, right? So they should match here.)

lidavidm · 2022-02-01T22:48:01Z

+ const std::vector<ValueDescr>& args) {
+ auto left_type = checked_cast<const TimestampType*>(args[0].type.get());
+ auto right_type = checked_cast<const TimestampType*>(args[1].type.get());
+ DCHECK_EQ(left_type->id(), right_type->id());


nit, but the checked_cast should account for this in debug mode since the dynamic_cast will give us nullptr (or else if we're going to assert, we should assert before casting)

lidavidm · 2022-02-01T22:48:39Z

+ RETURN_NOT_OK(
+ Status::Invalid("Subtraction of zoned and non-zoned times is ambiguous. (",
+ left_type->timezone(), right_type->timezone(), ")."));


nit, but just return Status::Invalid(...);?

ursabot · 2022-02-02T15:01:39Z

Benchmark runs are scheduled for baseline = c715beb and contender = 56386a4. 56386a4 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.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.09%] 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-actionsBot added the Component: C++ label Jan 11, 2022

rok force-pushed the ARROW-14093 branch from 755ca57 to ad971ecCompare January 11, 2022 16:21

lidavidm reviewed Jan 12, 2022
View reviewed changes

Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated

rok changed the title ~~ARROW-14093: [C++] subtract(date, date) -> interval kernel~~ARROW-14093: [C++] subtract(date, date) -> duration kernelJan 12, 2022

rok force-pushed the ARROW-14093 branch from ad971ec to f6ade6bCompare January 12, 2022 21:44

rok mentioned this pull request Jan 13, 2022
ARROW-14095: [C++] subtract(timestamp, duration) -> timestamp kernel #12137
Closed

rok force-pushed the ARROW-14093 branch from ef91a44 to 0470bc4Compare January 26, 2022 20:07

rok requested a review from lidavidm January 26, 2022 20:09

rok force-pushed the ARROW-14093 branch from 0470bc4 to 517a3a9Compare January 26, 2022 20:46

lidavidm reviewed Jan 28, 2022
View reviewed changes

Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

rok added 4 commits January 31, 2022 15:38

subtract(date, date)->duration
4c48850

Review feedback.
eae5350

Add ReplaceTemporalTypes
83ebaf1

Rebase for ARROW-14092
5b77577

rok force-pushed the ARROW-14093 branch 3 times, most recently from bb12021 to 970ac73Compare January 31, 2022 21:09

lidavidm self-requested a review January 31, 2022 22:36

lidavidm approved these changes Feb 1, 2022
View reviewed changes

Review feedback
f022b5e

rok force-pushed the ARROW-14093 branch from 970ac73 to f022b5eCompare February 1, 2022 14:52

lidavidm approved these changes Feb 1, 2022
View reviewed changes

Review feedback
66e1955

rok force-pushed the ARROW-14093 branch from 2138294 to 66e1955Compare February 2, 2022 11:30

lidavidm approved these changes Feb 2, 2022
View reviewed changes

lidavidm closed this in 56386a4Feb 2, 2022

Uh oh!

Conversation

rok commented Jan 11, 2022

Uh oh!

github-actionsBot commented Jan 11, 2022

Uh oh!

github-actionsBot commented Jan 11, 2022

Uh oh!

lidavidm commented Jan 12, 2022

Uh oh!

Uh oh!

rok commented Jan 12, 2022

Uh oh!

rok commented Jan 12, 2022

Uh oh!

lidavidm commented Jan 12, 2022

Uh oh!

lidavidm commented Jan 27, 2022

Uh oh!

rok commented Jan 27, 2022

Uh oh!

Uh oh!

Uh oh!

lidavidm left a comment

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!

Uh oh!

Uh oh!

rok commented Feb 1, 2022

Uh oh!

lidavidm left a comment

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!

ursabot commented Feb 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ursabot commented Feb 2, 2022•
edited
Loading