You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/11 20:03:57 UTC
[GitHub] [arrow] rok opened a new pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
rok opened a new pull request #12612:
URL: https://github.com/apache/arrow/pull/12612
This is to resolve [ARROW-15919](https://issues.apache.org/jira/browse/ARROW-15919).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829361298
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2637,9 +2609,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
Review comment:
Done.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2661,9 +2637,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add_checked(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1067110733
@pitrou could you please take a look at this?
I'm afraid it's not very pretty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829012588
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Done.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2637,9 +2609,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
Review comment:
Done.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2661,9 +2637,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add_checked(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
Review comment:
Done.
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1070,6 +1070,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
CheckScalarBinary(op, dates32, durations_us, timestamps_us);
CheckScalarBinary(op, dates64, durations_us, timestamps_us);
+
+ CheckScalarBinary(op, durations_us, dates32, timestamps_us);
+ CheckScalarBinary(op, durations_us, dates64, timestamps_us);
Review comment:
Nice! Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1073922131
Benchmark runs are scheduled for baseline = e258e1c5a716c141b44fcab7a039cfad627674d4 and contender = 70b8a8248fb573178b50bfde12f3a992025d610c. 70b8a8248fb573178b50bfde12f3a992025d610c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b2330448eaa4a40b4f0e3394b8da456...c6f7e7c4104f481e8d29ed806a12fa9f/)
[Finished :arrow_down:0.5% :arrow_up:0.08%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eff00181adbc4e87ab37e9dea58fbe7e...2a3a42cdda354ea49bb868d24d9ac082/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/39974ab51768453cb68c27077677e8f7...a17910f17c9e47178c261694f9c2689d/)
[Finished :arrow_down:0.68% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/129aaf5c69ae4229a021783d2c64e41c...dcc1b116e428498ca77fe07dbadffdea/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1065462901
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r828507912
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Done. Would you suggest to do this for `SubtractTimeDuration` and `SubtractTimeDurationChecked` as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829179079
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2637,9 +2609,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
Review comment:
As a nit, this is the same as `exec1`, so you can just remove the `std::move` instead of recreating a new one :-)
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1070,6 +1070,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
CheckScalarBinary(op, dates32, durations_us, timestamps_us);
CheckScalarBinary(op, dates64, durations_us, timestamps_us);
+
+ CheckScalarBinary(op, durations_us, dates32, timestamps_us);
+ CheckScalarBinary(op, durations_us, dates64, timestamps_us);
Review comment:
Here's a thought: how about we define `CheckScalarBinaryCommutative` so that we can simply replace `CheckScalarBinary` with it in all heterogenous addition tests?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2661,9 +2637,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add_checked(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
Review comment:
Same here as well of course (and probably below?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1073922131
Benchmark runs are scheduled for baseline = e258e1c5a716c141b44fcab7a039cfad627674d4 and contender = 70b8a8248fb573178b50bfde12f3a992025d610c. 70b8a8248fb573178b50bfde12f3a992025d610c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b2330448eaa4a40b4f0e3394b8da456...c6f7e7c4104f481e8d29ed806a12fa9f/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eff00181adbc4e87ab37e9dea58fbe7e...2a3a42cdda354ea49bb868d24d9ac082/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/39974ab51768453cb68c27077677e8f7...a17910f17c9e47178c261694f9c2689d/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/129aaf5c69ae4229a021783d2c64e41c...dcc1b116e428498ca77fe07dbadffdea/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1073922131
Benchmark runs are scheduled for baseline = e258e1c5a716c141b44fcab7a039cfad627674d4 and contender = 70b8a8248fb573178b50bfde12f3a992025d610c. 70b8a8248fb573178b50bfde12f3a992025d610c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b2330448eaa4a40b4f0e3394b8da456...c6f7e7c4104f481e8d29ed806a12fa9f/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eff00181adbc4e87ab37e9dea58fbe7e...2a3a42cdda354ea49bb868d24d9ac082/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/39974ab51768453cb68c27077677e8f7...a17910f17c9e47178c261694f9c2689d/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/129aaf5c69ae4229a021783d2c64e41c...dcc1b116e428498ca77fe07dbadffdea/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1073922131
Benchmark runs are scheduled for baseline = e258e1c5a716c141b44fcab7a039cfad627674d4 and contender = 70b8a8248fb573178b50bfde12f3a992025d610c. 70b8a8248fb573178b50bfde12f3a992025d610c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b2330448eaa4a40b4f0e3394b8da456...c6f7e7c4104f481e8d29ed806a12fa9f/)
[Finished :arrow_down:0.5% :arrow_up:0.08%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eff00181adbc4e87ab37e9dea58fbe7e...2a3a42cdda354ea49bb868d24d9ac082/)
[Finished :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/39974ab51768453cb68c27077677e8f7...a17910f17c9e47178c261694f9c2689d/)
[Finished :arrow_down:0.68% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/129aaf5c69ae4229a021783d2c64e41c...dcc1b116e428498ca77fe07dbadffdea/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829012588
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829361698
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1070,6 +1070,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
CheckScalarBinary(op, dates32, durations_us, timestamps_us);
CheckScalarBinary(op, dates64, durations_us, timestamps_us);
+
+ CheckScalarBinary(op, durations_us, dates32, timestamps_us);
+ CheckScalarBinary(op, durations_us, dates64, timestamps_us);
Review comment:
Nice! Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r829179079
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2637,9 +2609,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
Review comment:
As a nit, this is the same as `exec1`, so you can just remove the `std::move` instead of recreating a new one :-)
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1070,6 +1070,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
CheckScalarBinary(op, dates32, durations_us, timestamps_us);
CheckScalarBinary(op, dates64, durations_us, timestamps_us);
+
+ CheckScalarBinary(op, durations_us, dates32, timestamps_us);
+ CheckScalarBinary(op, durations_us, dates64, timestamps_us);
Review comment:
Here's a thought: how about we define `CheckScalarBinaryCommutative` so that we can simply replace `CheckScalarBinary` with it in all heterogenous addition tests?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2661,9 +2637,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
// Add add_checked(timestamp, duration) -> timestamp
for (auto unit : TimeUnit::values()) {
InputType in_type(match::TimestampTypeUnit(unit));
- auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+ auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
- std::move(exec)));
+ std::move(exec1)));
+ auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
Review comment:
Same here as well of course (and probably below?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r827246795
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Why not `arrow::internal::SafeSignedAdd(static_cast<T>(left), static_cast<T>(right))` and then you don't have to bother with different signatures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r827314855
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Would double cast not be wasteful? Will change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12612: ARROW-15919: [C++] Add_kernel not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#discussion_r828972563
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
}
};
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
struct AddTimeDuration {
template <typename T, typename Arg0, typename Arg1>
- static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
- Status* st) {
+ static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+ KernelContext*, Arg0 left, Arg1 right, Status* st) {
T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
Review comment:
Everywhere it's applicable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12612:
URL: https://github.com/apache/arrow/pull/12612#issuecomment-1073922131
Benchmark runs are scheduled for baseline = e258e1c5a716c141b44fcab7a039cfad627674d4 and contender = 70b8a8248fb573178b50bfde12f3a992025d610c. 70b8a8248fb573178b50bfde12f3a992025d610c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b2330448eaa4a40b4f0e3394b8da456...c6f7e7c4104f481e8d29ed806a12fa9f/)
[Finished :arrow_down:0.5% :arrow_up:0.08%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eff00181adbc4e87ab37e9dea58fbe7e...2a3a42cdda354ea49bb868d24d9ac082/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/39974ab51768453cb68c27077677e8f7...a17910f17c9e47178c261694f9c2689d/)
[Finished :arrow_down:0.68% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/129aaf5c69ae4229a021783d2c64e41c...dcc1b116e428498ca77fe07dbadffdea/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #12612: ARROW-15919: [C++] Add function not commutative with timestamps & duration maths
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #12612:
URL: https://github.com/apache/arrow/pull/12612
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org