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/28 11:30:15 UTC
[GitHub] [arrow] milesgranger opened a new pull request, #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
milesgranger opened a new pull request, #14749:
URL: https://github.com/apache/arrow/pull/14749
Will close [ARROW-18280](https://issues.apache.org/jira/browse/ARROW-18280)
--
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 merged pull request #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche merged PR #14749:
URL: https://github.com/apache/arrow/pull/14749
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1034395351
##########
cpp/src/arrow/compute/kernels/scalar_nested_test.cc:
##########
@@ -168,6 +173,20 @@ TEST(TestScalarNested, ListSliceFixedOutput) {
expected = ArrayFromJSON(fixed_size_list(value_type, 2),
"[[3, null], [null, null], [null, null], null]");
CheckScalarUnary("list_slice", input, expected, &args);
+
+ args.start = 1;
+ args.stop = std::nullopt;
+ expected = ArrayFromJSON(fixed_size_list(value_type, 2),
+ "[[2, 3], [5, null], [null, null], null]");
+ if (input->type()->id() == Type::FIXED_SIZE_LIST) {
+ CheckScalarUnary("list_slice", input, expected, &args);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ NotImplemented,
+ ::testing::HasSubstr("Unable to produce FixedSizeListArray from "
+ "non-FixedSizeListArray without `stop` being set."),
+ CallFunction("list_slice", {input}, &args));
+ }
Review Comment:
That can indeed be possible, and a nice extra test case. Thank you!
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14749:
URL: https://github.com/apache/arrow/pull/14749#issuecomment-1339294886
Benchmark runs are scheduled for baseline = 4daddd7cc31325ce34bb8b0fde562dba6ee3c731 and contender = 373812d0aa2c8b1238ad8ed13c8a173fc2fef710. 373812d0aa2c8b1238ad8ed13c8a173fc2fef710 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/3247ebb0408f4fe1b8f4ea0330131568...61027ed4139a4b63a3c106d4ab566cfb/)
[Finished :arrow_down:0.13% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/7c7ec00c7144467999d4307fcf9cead9...c7ad1b71b663420f9257e5ce9257381a/)
[Finished :arrow_down:0.27% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/50973abac0d641669722126a6e294c4f...350bfed89647456fb403ca71ddd84f5c/)
[Finished :arrow_down:0.52% :arrow_up:0.07%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/731a143b43694210a096276d458f6f15...a6f1487eb64047359dc23988815d059d/)
Buildkite builds:
[Finished] [`373812d0` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1978)
[Finished] [`373812d0` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2000)
[Finished] [`373812d0` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1970)
[Finished] [`373812d0` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1992)
[Finished] [`4daddd7c` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1977)
[Finished] [`4daddd7c` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1999)
[Finished] [`4daddd7c` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1969)
[Finished] [`4daddd7c` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1991)
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1035969438
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return reinterpret_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
Review Comment:
I am no C++ expert, but on my eyes it seems a bit overly complicated for a simple if/else ;)
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1034391074
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return checked_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
+ const auto size = std::max(stop - static_cast<int32_t>(opts.start), 0);
Review Comment:
This covers the case of your suggested case where `stop` is null and the fixed size list stop is then set to the list_size, which could be less than the start.
--
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] wjones127 commented on a diff in pull request #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1034147079
##########
cpp/src/arrow/compute/kernels/scalar_nested_test.cc:
##########
@@ -168,6 +173,20 @@ TEST(TestScalarNested, ListSliceFixedOutput) {
expected = ArrayFromJSON(fixed_size_list(value_type, 2),
"[[3, null], [null, null], [null, null], null]");
CheckScalarUnary("list_slice", input, expected, &args);
+
+ args.start = 1;
+ args.stop = std::nullopt;
+ expected = ArrayFromJSON(fixed_size_list(value_type, 2),
+ "[[2, 3], [5, null], [null, null], null]");
+ if (input->type()->id() == Type::FIXED_SIZE_LIST) {
+ CheckScalarUnary("list_slice", input, expected, &args);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ NotImplemented,
+ ::testing::HasSubstr("Unable to produce FixedSizeListArray from "
+ "non-FixedSizeListArray without `stop` being set."),
+ CallFunction("list_slice", {input}, &args));
+ }
Review Comment:
Is this a case that is possible?
```suggestion
}
args.start = 3;
args.stop = std::nullopt;
expected = ArrayFromJSON(fixed_size_list(value_type, 0),
"[[], [], [], null]");
if (input->type()->id() == Type::FIXED_SIZE_LIST) {
CheckScalarUnary("list_slice", input, expected, &args);
}
```
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return checked_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
+ const auto size = std::max(stop - static_cast<int32_t>(opts.start), 0);
Review Comment:
Isn't this guaranteed to be non-negative?
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14749:
URL: https://github.com/apache/arrow/pull/14749#issuecomment-1339297118
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/50973abac0d641669722126a6e294c4f...350bfed89647456fb403ca71ddd84f5c/)
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14749:
URL: https://github.com/apache/arrow/pull/14749#issuecomment-1328925017
https://issues.apache.org/jira/browse/ARROW-18280
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1035916807
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return reinterpret_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
Review Comment:
...because one can declare `stop` as a `const` for the cost of one extra line? haha.
It actually spawned from my trying to satisfy MinGW thnking I was illegally casting a different ListType than `FixedSizeList` in the second arm.. However, in retrospect I think that was because I was using the ternary operator `condition ? expr : expr` which MinGW apparently would type check both expressions.
Anyway, I can remove the lambda if that's preferred.
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1035805252
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return reinterpret_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
Review Comment:
Code question: is there a reason this is done as a lambda, instead of just a if/else block? (like you have in the resolver)
--
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 #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1035805252
##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
list_type->id() == arrow::Type::FIXED_SIZE_LIST);
std::unique_ptr<ArrayBuilder> builder;
+ // should have been checked in resolver
+ // if stop not set, then cannot return fixed size list without input being fixed size
+ // list b/c we cannot determine the max list element in type resolving.
+ DCHECK(opts.stop.has_value() ||
+ (!opts.stop.has_value() && (!return_fixed_size_list ||
+ list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
// construct array values
if (return_fixed_size_list) {
- RETURN_NOT_OK(MakeBuilder(
- ctx->memory_pool(),
- fixed_size_list(value_type,
- static_cast<int32_t>(opts.stop.value() - opts.start)),
- &builder));
+ const auto stop = [&]() {
+ if (opts.stop.has_value()) {
+ return static_cast<int32_t>(opts.stop.value());
+ } else {
+ DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+ return reinterpret_cast<const FixedSizeListType*>(list_type)->list_size();
+ }
+ }();
Review Comment:
Code question: is there a reason this is done as a lambda, instead of just a if/else block?
--
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