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/01/13 10:30:36 UTC
[GitHub] [arrow] rok opened a new pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
rok opened a new pull request #12139:
URL: https://github.com/apache/arrow/pull/12139
[ARROW-14097](https://issues.apache.org/jira/browse/ARROW-14097)
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r794702513
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_temporal.cc
##########
@@ -519,6 +521,16 @@ std::shared_ptr<CastFunction> GetDurationCast() {
// Between durations
AddCrossUnitCast<DurationType>(func.get());
+ // time32/64 -> duration
Review comment:
Indeed. Removed.
--
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] lidavidm commented on pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1030352188
Sorry, I'll take a look at this today or Monday
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r795725075
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
Huh. I wonder how expected this would be. But you're proposing something like this:
```
if (result < 0) {
result = result % multiple;
}
```
Correct?
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1012002939
https://issues.apache.org/jira/browse/ARROW-14097
--
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] lidavidm commented on a change in pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r800081066
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
I found this reference: https://bugs.python.org/issue1487389#msg54803
I think raising an error is OK. If we need wrap-around, we can add that as a separate kernel perhaps.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
We should still raise an error if the result is >24:00 though.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r794702370
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2437,6 +2458,30 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
std::move(exec)));
}
+ // Add subtract(duration, duration) -> duration
+ for (auto unit : TimeUnit::values()) {
+ InputType in_type(match::DurationTypeUnit(unit));
+ auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Subtract>(Type::DURATION);
+ DCHECK_OK(subtract->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
+ }
+
+ // Add subtract(time32, duration) -> duration
Review comment:
Yeah switching to `subtract(time32/64, duration) -> time32/64`. I suppose validation should a be part of the subtract kernel then.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r795717545
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2090,6 +2146,34 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionFloatingPointNotNull(
return func;
}
+template <template <bool, int64_t> class Op>
+void AddArithmeticFunctionTimeDurations(std::shared_ptr<ScalarFunction> func) {
+ // Add subtract(time32, duration) -> time32
+ TimeUnit::type unit = TimeUnit::SECOND;
+ auto exec_1 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time32TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_1)));
+
+ unit = TimeUnit::MILLI;
+ auto exec_2 =
+ ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time32TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_2)));
+
+ // Add subtract(time64, duration) -> time64
+ unit = TimeUnit::MICRO;
+ auto exec_3 =
+ ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time64TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_3)));
+
+ unit = TimeUnit::NANO;
+ auto exec_4 =
+ ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time64TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_4)));
Review comment:
Good point!
--
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] lidavidm commented on a change in pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r798024695
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
I poked around stdlib datetime but it doesn't seem to implement this. However, I still think we need to check whether the result is in range on both ends.
--
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] lidavidm commented on pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032603741
Looks like the linter is unhappy here.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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] rok commented on pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1030361201
> Sorry, I'll take a look at this today or Monday
No worries, this is on me anyway :)
--
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] lidavidm commented on a change in pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r795705061
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2090,6 +2146,34 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionFloatingPointNotNull(
return func;
}
+template <template <bool, int64_t> class Op>
+void AddArithmeticFunctionTimeDurations(std::shared_ptr<ScalarFunction> func) {
+ // Add subtract(time32, duration) -> time32
+ TimeUnit::type unit = TimeUnit::SECOND;
+ auto exec_1 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time32TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_1)));
+
+ unit = TimeUnit::MILLI;
+ auto exec_2 =
+ ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time32TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_2)));
+
+ // Add subtract(time64, duration) -> time64
+ unit = TimeUnit::MICRO;
+ auto exec_3 =
+ ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time64TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_3)));
+
+ unit = TimeUnit::NANO;
+ auto exec_4 =
+ ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000000>>::Exec;
+ DCHECK_OK(func->AddKernel({match::Time64TypeUnit(unit), duration(unit)},
+ OutputType(FirstType), std::move(exec_4)));
Review comment:
nit, but is there any advantage to using the type matcher + computed output type vs the literal types?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
Since we could subtract a negative duration, we should also check if the output is larger than the maximum acceptable value.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
Alternatively - maybe this arithmetic should wrap around? Does that make any sense?
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r795728909
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
It feels semantically correct but I'm going to check what Pandas and others do to see what conventions are.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r794941782
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2437,6 +2458,30 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
std::move(exec)));
}
+ // Add subtract(duration, duration) -> duration
+ for (auto unit : TimeUnit::values()) {
+ InputType in_type(match::DurationTypeUnit(unit));
+ auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Subtract>(Type::DURATION);
+ DCHECK_OK(subtract->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
+ }
+
+ // Add subtract(time32, duration) -> duration
Review comment:
Added tests.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Failed] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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] lidavidm closed pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #12139:
URL: https://github.com/apache/arrow/pull/12139
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Finished :arrow_down:0.77% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Finished :arrow_down:0.65% :arrow_up:0.17%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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] lidavidm commented on a change in pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r794561816
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_temporal.cc
##########
@@ -519,6 +521,16 @@ std::shared_ptr<CastFunction> GetDurationCast() {
// Between durations
AddCrossUnitCast<DurationType>(func.get());
+ // time32/64 -> duration
Review comment:
IMO, it doesn't make sense to cast time to duration.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2437,6 +2458,30 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
std::move(exec)));
}
+ // Add subtract(duration, duration) -> duration
+ for (auto unit : TimeUnit::values()) {
+ InputType in_type(match::DurationTypeUnit(unit));
+ auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Subtract>(Type::DURATION);
+ DCHECK_OK(subtract->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
+ }
+
+ // Add subtract(time32, duration) -> duration
Review comment:
This doesn't match the PR title, and time makes more sense than duration as a result right?
Also, in that case, we need to validate that the result is not <0 or >=24h (see #12014).
--
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] lidavidm commented on a change in pull request #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r795726185
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
Right. Though, I'm not sure if that really makes any sense to have.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r794910814
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2437,6 +2458,30 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
std::move(exec)));
}
+ // Add subtract(duration, duration) -> duration
+ for (auto unit : TimeUnit::values()) {
+ InputType in_type(match::DurationTypeUnit(unit));
+ auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Subtract>(Type::DURATION);
+ DCHECK_OK(subtract->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
+ }
+
+ // Add subtract(time32, duration) -> duration
Review comment:
Added the validation. Still needs tests.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
rok commented on a change in pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#discussion_r801534362
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -214,6 +214,62 @@ struct SubtractChecked {
}
};
+template <bool is_32bit, int64_t multiple>
+struct SubtractTimeDuration {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status* st) {
+ T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
+ if (result < 0) {
+ *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+ multiple, ") s");
+ }
Review comment:
Thanks for looking into this! Pandas appears delegate time to Python and rather work with timedelta which is like our duration.
I made this raise on time < 0 and time >= 24:00. Lets revisit if/when desired.
--
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Finished :arrow_down:0.65% :arrow_up:0.17%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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 #12139: ARROW-14097: [C++] subtract(time, duration) -> time kernel
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12139:
URL: https://github.com/apache/arrow/pull/12139#issuecomment-1032748807
Benchmark runs are scheduled for baseline = 49d2a41280e191bb95f76595e7eb68958cc03990 and contender = 11e9f269164539cfd57a10e545c1fb3aeb19c711. 11e9f269164539cfd57a10e545c1fb3aeb19c711 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/efd7e624ddba4f1aa0eb3eef0ea40168...bbf6f5a0cadd4f75a18d7b539e1f7ccd/)
[Finished :arrow_down:0.77% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/651a0e265f994f6ab0c2a947703cdc3e...945ac340f4df473286823667c57994cf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/168d220db887463c9d087d5d4c2d2474...25bf21eff25548979a91aef2f3c168b8/)
[Finished :arrow_down:0.65% :arrow_up:0.17%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/918e429f2ec24e2999e48a367866881c...895f4fd849804960ab3e5769ec47e8d0/)
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