jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035749359
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,22 @@ std::string ToString(const std::optional<T>& o) {
return o.has_value() ? ToChars(*o) : "(nullopt)";
}
+int64_t MaxSliceLength(const int64_t start, const int64_t stop, const int64_t
step) {
+ const auto startf = static_cast<float>(start);
+ const auto stopf = static_cast<float>(stop);
+ const auto stepf = static_cast<float>(step);
+
+ if (stopf - startf <= stepf) {
+ return 1;
+ }
+
+ auto length = static_cast<int64_t>(std::floor((stopf - startf) / stepf));
Review Comment:
Small nit: you can maybe do the casting to int64 on the return line, so yo
don't need to cast back to float on the next line?
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,22 @@ std::string ToString(const std::optional<T>& o) {
return o.has_value() ? ToChars(*o) : "(nullopt)";
}
+int64_t MaxSliceLength(const int64_t start, const int64_t stop, const int64_t
step) {
+ const auto startf = static_cast<float>(start);
+ const auto stopf = static_cast<float>(stop);
+ const auto stepf = static_cast<float>(step);
+
+ if (stopf - startf <= stepf) {
+ return 1;
+ }
+
+ auto length = static_cast<int64_t>(std::floor((stopf - startf) / stepf));
+ if (fmod(static_cast<float>(length), stepf) > 0.0) {
+ ++length;
+ }
+ return length;
Review Comment:
Can you add some more tests with varying start/stop/step values that cover
the different cases that this helper function needs to handle? (where the
flooring actually does something or not, where the fmod is zero or not, ..)
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,22 @@ std::string ToString(const std::optional<T>& o) {
return o.has_value() ? ToChars(*o) : "(nullopt)";
}
+int64_t MaxSliceLength(const int64_t start, const int64_t stop, const int64_t
step) {
+ const auto startf = static_cast<float>(start);
+ const auto stopf = static_cast<float>(stop);
+ const auto stepf = static_cast<float>(step);
+
+ if (stopf - startf <= stepf) {
+ return 1;
+ }
+
+ auto length = static_cast<int64_t>(std::floor((stopf - startf) / stepf));
+ if (fmod(static_cast<float>(length), stepf) > 0.0) {
+ ++length;
+ }
+ return length;
Review Comment:
I also don't understand anymore why we are exactly doing the `fmod(length,
step)`, so a small comment might be helpful.
(also, shouldn't it be `fmod(stop-start, step)` instead?)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
