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/11/22 12:12:22 UTC
[GitHub] [arrow] milesgranger opened a new pull request, #14696: Support step >= 1 in list_slice kernel
milesgranger opened a new pull request, #14696:
URL: https://github.com/apache/arrow/pull/14696
Will fix [ARROW-18282](https://issues.apache.org/jira/browse/ARROW-18282)
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1036382227
##########
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:
Aye, good eye! And that exposed a slight buggy impl of the max slice helper. Too early in the flooring for length. :pray:
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035902151
##########
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:
More tests added, let me know if you see any specific combinations you'd like to see.
> (also, shouldn't it be fmod(stop-start, step) instead?)
It needs to be `length`, which if there is no step (rather when step==1), then `stop-start` is okay, but that divided by step gives the actual length if step != 1. Then (at least in python) if there is any remainder one more is added to the possible length.
--
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] milesgranger commented on pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on PR #14696:
URL: https://github.com/apache/arrow/pull/14696#issuecomment-1330732263
Gentle ping @jorisvandenbossche
I don't think the [failing R / AMD64 Windows C++ RTools](https://github.com/apache/arrow/actions/runs/3574525679/jobs/6009928351#step:7:3601) is related.
--
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 #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14696:
URL: https://github.com/apache/arrow/pull/14696#issuecomment-1336263197
Benchmark runs are scheduled for baseline = 53abebb06f511a8980d03abbf56b614929dea0a4 and contender = 54ff2d8777717ea5bb811f3653deeb12fc93452e. 54ff2d8777717ea5bb811f3653deeb12fc93452e 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/cc383a945b1e481eb9818d5795507a62...b49e8f5cdf1c46ef8881806df41f23bb/)
[Failed :arrow_down:0.1% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65dc234eaa0d462db276ed3d9c945062...5a8b970159464950a380d3d993dc3f33/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b61333402e5143f4a7c44f7cbe9f069c...940a48925f9a432987f2ed64f7cd9861/)
[Finished :arrow_down:1.12% :arrow_up:0.07%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/e9b04dea1de648cf8c6db98760e41756...3f6f534be32941e9944e992c71067bcb/)
Buildkite builds:
[Finished] [`54ff2d87` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1968)
[Failed] [`54ff2d87` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1990)
[Finished] [`54ff2d87` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1960)
[Finished] [`54ff2d87` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1982)
[Finished] [`53abebb0` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1967)
[Failed] [`53abebb0` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1989)
[Finished] [`53abebb0` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1959)
[Finished] [`53abebb0` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1981)
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] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035980132
##########
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:
> More tests added, let me know if you see any specific combinations you'd like to see.
I think you will need to add a longer test list to cover all cases. For example the (1, 3, 2) and (0, 2, 3) cases you added both already shortcut in the `if (stopf - startf <= stepf) { return 1; }` case, and so don't actually cover the floor/fmod logic.
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035908954
##########
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:
https://github.com/apache/arrow/pull/14696/commits/953e4863137680bbdc37bbd6bf5c37edf8991838
--
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] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1036798132
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,16 @@ 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);
+
+ // Get the raw length with any remainder after dividing by step
+ auto length = std::ceil((stopf - startf) / stepf);
+ return static_cast<int64_t>(std::floor(length));
Review Comment:
```suggestion
return static_cast<int64_t>(length);
```
--
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 #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14696:
URL: https://github.com/apache/arrow/pull/14696#issuecomment-1323569853
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035983534
##########
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:
For example, can you test (0, 5, 2) and (0, 6, 3)?
--
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 diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1038282327
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,15 @@ 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);
+
+ auto length = std::ceil((stopf - startf) / stepf);
+ return static_cast<int64_t>(length);
+}
Review Comment:
Instead of floats, can we use CeilDiv from arrow/util/bit_util.h?
--
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] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1036764141
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,28 @@ 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);
+
+ // length not considering step, is less than step then
+ // the length will be 1; the index of start.
+ if (stopf - startf <= stepf) {
+ return 1;
+ }
+
+ // Get the raw length with any remainder after dividing by step
+ auto length = (stopf - startf) / stepf;
+
+ // Any remainder from the step into length then it is not evenly split by step
+ // and will need one extra in length.
+ if (length > std::floor(length)) {
+ ++length;
+ }
Review Comment:
I am wondering, isn't this now the same as rounding up? (std::ceil)
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1038714941
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,15 @@ 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);
+
+ auto length = std::ceil((stopf - startf) / stepf);
+ return static_cast<int64_t>(length);
+}
Review Comment:
ah, had to catch the the divisor (step) being >= 1 before the call in the resolver. Updated in https://github.com/apache/arrow/pull/14696/commits/ad6d055496a5204bba05454c076f25df24dfb01e
--
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 #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14696:
URL: https://github.com/apache/arrow/pull/14696#issuecomment-1323570232
https://issues.apache.org/jira/browse/ARROW-18282
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035902151
##########
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:
More tests added, let me know if you see any specific combinations you'd like to see.
> (also, shouldn't it be fmod(stop-start, step) instead?)
It needs to be `length`, which if there is no step (rather when step==1), then `stop-start` is okay, but that divided by step gives the actual length if step != 1. Then if there is any remainder one more is added to the possible length.
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1035902151
##########
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:
More tests added, let me know if you see any specific combinations you'd like to see.
> (also, shouldn't it be fmod(stop-start, step) instead?)
It needs to be `length`, which if there is no step, then `stop-start` is okay, but that divided by step gives the actual length if step != 1. Then (at least in python) if there is any remainder one more is added to the possible length.
--
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] jorisvandenbossche commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1037892212
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,16 @@ 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);
+
+ // Get the raw length with any remainder after dividing by step
Review Comment:
I think this comment was from before ceiling, and is now a bit confusing?
--
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] milesgranger commented on a diff in pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14696:
URL: https://github.com/apache/arrow/pull/14696#discussion_r1038298253
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -96,6 +97,15 @@ 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);
+
+ auto length = std::ceil((stopf - startf) / stepf);
+ return static_cast<int64_t>(length);
+}
Review Comment:
Hmm, I get a `Fatal Python error: Floating point exception` locally, when switching to that. Will try again later this weekend/early next week.
--
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 merged pull request #14696: ARROW-18282: [C++][Python] Support step >= 1 in list_slice kernel
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #14696:
URL: https://github.com/apache/arrow/pull/14696
--
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