ARROW-14101: [C++] multiply(duration, integer) -> duration kernel #12215

rok · 2022-01-20T18:21:00Z

This is to resolve ARROW-14101 and ARROW-14102.

github-actions · 2022-01-20T18:27:48Z

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

jorisvandenbossche · 2022-01-27T15:18:18Z

For the mutiply_checked, should we add a test for a duration that would go out of bounds? Or is this already sufficiently covered by integer multiply_checked tests if that is using the same underlying implementation?

rok · 2022-01-27T15:51:06Z

For the mutiply_checked, should we add a test for a duration that would go out of bounds? Or is this already sufficiently covered by integer multiply_checked tests if that is using the same underlying implementation?

Another test won't hurt :) I'll add it.

rok · 2022-01-27T19:41:55Z

Added the test and removed divide checked (seemed redundant since we're only dividing with integers).

rok · 2022-02-18T12:59:22Z

@pitrou could you please take a look at this one? It covers multiply and divide (ARROW-14102).

I think the CI error is due to s3fs.

jorisvandenbossche · 2022-02-22T08:21:57Z

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

Can you have a case in this test where the division is not exact?

docs/source/cpp/compute.rst

jorisvandenbossche · 2022-02-22T08:23:42Z

Added the test and removed divide checked (seemed redundant since we're only dividing with integers).

I would keep divide_checked. You could give the same argument for divide_checked being redundant for integer types, but we still allow you to use it. It could be annoying that, when using divide_checked, you need to switch to divide, only if your input type is duration.

rok · 2022-02-22T13:28:58Z

Thanks for reviewing @jorisvandenbossche. Agreed on the divide_checked. Added.

pitrou · 2022-02-22T13:52:54Z

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

This looks unexpected to me since int64 is not a temporal at all. @lidavidm What do you think?

I think this is fine, but maybe we need a better name for what this function does.

That said, I don't think this change is needed for this function to work, anyways. There's only a single temporal argument so there isn't really a meaningful common resolution to cast to.

To be honest, the function doesn't seem to have changed :-). It's just the additional test that surprised me (but adding a test for existing behaviour is useful, thank you @rok ).

Well duration has a unit and int64 is dimensionless. The goal of the function is to find the best common unit and I'd argue it still does.

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

pitrou · 2022-02-22T13:56:46Z

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

Is it worth also supporting multiply(int64, duration) or would commutativity be handled at an upper level? @lidavidm

Hmm, I don't see why not. It could just use the same exec function.

pitrou · 2022-02-22T13:57:51Z

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

Can you some negative value(s) into the mix?

pitrou · 2022-02-22T13:58:41Z

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

Also check that division by zero raises?

ursabot · 2022-02-23T14:41:38Z

Benchmark runs are scheduled for baseline = 194ace5 and contender = 2f8f1e8. 2f8f1e8 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.25% ⬆️0.13%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.38% ⬆️0.21%] 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

rok · 2022-02-23T15:05:26Z

Thanks all!

rok changed the title ~~ARROW-14101: [C++] multiply(interval, integer) -> interval kernel~~ ARROW-14101: [C++] multiply(duration, integer) -> duration kernel Jan 20, 2022

github-actions bot added the Component: C++ label Jan 20, 2022

rok force-pushed the ARROW-14101 branch from 3ffd781 to 8c2ffd3 Compare January 26, 2022 21:14

rok force-pushed the ARROW-14101 branch 3 times, most recently from 1fd50f0 to 9f40218 Compare January 27, 2022 19:40

rok force-pushed the ARROW-14101 branch 2 times, most recently from c27af81 to 6e4f4bd Compare February 2, 2022 17:55

rok force-pushed the ARROW-14101 branch from 6e4f4bd to 6eb0e97 Compare February 8, 2022 13:56

rok mentioned this pull request Feb 8, 2022

ARROW-14100: [C++] subtract(duration, duration) -> duration kernel #12141

Closed

rok force-pushed the ARROW-14101 branch 3 times, most recently from 7ad426d to 33d1458 Compare February 9, 2022 19:23

rok force-pushed the ARROW-14101 branch 2 times, most recently from d4fdb74 to 5c015d4 Compare February 17, 2022 17:35

rok force-pushed the ARROW-14101 branch from 5c015d4 to 45b12c8 Compare February 21, 2022 04:06

jorisvandenbossche reviewed Feb 22, 2022

View reviewed changes

docs/source/cpp/compute.rst Outdated Show resolved Hide resolved

rok force-pushed the ARROW-14101 branch from 45b12c8 to eea7979 Compare February 22, 2022 13:26

pitrou reviewed Feb 22, 2022

View reviewed changes

multiply(duration, integer) -> duration

e3fa8fe

rok force-pushed the ARROW-14101 branch from f2aa73b to dc14cfe Compare February 22, 2022 15:58

Review feedback

c797d52

rok force-pushed the ARROW-14101 branch from dc14cfe to c797d52 Compare February 23, 2022 02:06

rok requested a review from pitrou February 23, 2022 11:07

pitrou approved these changes Feb 23, 2022

View reviewed changes

pitrou closed this in 2f8f1e8 Feb 23, 2022

rok mentioned this pull request Nov 17, 2025

[C++][Python] Add multiply/divide kernels for duration and float/double #48003

Open

ARROW-14101: [C++] multiply(duration, integer) -> duration kernel #12215

ARROW-14101: [C++] multiply(duration, integer) -> duration kernel #12215

Uh oh!

Conversation

rok commented Jan 20, 2022

Uh oh!

github-actions bot commented Jan 20, 2022

Uh oh!

jorisvandenbossche commented Jan 27, 2022

Uh oh!

rok commented Jan 27, 2022

Uh oh!

rok commented Jan 27, 2022

Uh oh!

rok commented Feb 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorisvandenbossche commented Feb 22, 2022

Uh oh!

rok commented Feb 22, 2022

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!

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 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rok commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ursabot commented Feb 23, 2022 •

edited

Loading