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 2021/05/19 19:13:02 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
lidavidm opened a new pull request #10358:
URL: https://github.com/apache/arrow/pull/10358
Add a simple index() kernel.
Note that the Python start/end options are handled entirely in Python, not in the kernel itself.
Short-circuiting is somewhat implemented: the kernel executor will still loop through all batches but the kernel will stop looking at data once it finds an item.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#discussion_r636038852
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -218,6 +218,65 @@ Result<std::unique_ptr<KernelState>> AllInit(KernelContext*, const KernelInitArg
return ::arrow::internal::make_unique<BooleanAllImpl>();
}
+// ----------------------------------------------------------------------
+// Index implementation
+
+struct IndexImpl : public ScalarAggregator {
+ explicit IndexImpl(IndexOptions options, int64_t seen, int64_t index)
+ : options(std::move(options)), seen{seen}, index{index} {}
+
+ Status Consume(KernelContext* ctx, const ExecBatch& batch) override {
+ // short-circuit
+ if (index >= 0 || !options.value->is_valid) {
+ return Status::OK();
+ }
+
+ const auto& data = *batch[0].array();
+ seen = data.length;
+ ARROW_ASSIGN_OR_RAISE(
+ auto result, CallFunction("equal", {data, options.value}, ctx->exec_context()));
Review comment:
Yup, that is fair. I figured for now I didn't want to create a lot of generated code for this use case. It also at least helps that it'll short-circuit.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#discussion_r635964852
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -218,6 +218,65 @@ Result<std::unique_ptr<KernelState>> AllInit(KernelContext*, const KernelInitArg
return ::arrow::internal::make_unique<BooleanAllImpl>();
}
+// ----------------------------------------------------------------------
+// Index implementation
+
+struct IndexImpl : public ScalarAggregator {
+ explicit IndexImpl(IndexOptions options, int64_t seen, int64_t index)
+ : options(std::move(options)), seen{seen}, index{index} {}
+
+ Status Consume(KernelContext* ctx, const ExecBatch& batch) override {
+ // short-circuit
+ if (index >= 0 || !options.value->is_valid) {
+ return Status::OK();
+ }
+
+ const auto& data = *batch[0].array();
+ seen = data.length;
+ ARROW_ASSIGN_OR_RAISE(
+ auto result, CallFunction("equal", {data, options.value}, ctx->exec_context()));
Review comment:
Looks not very efficient as "equal" scans the whole input array (and builds a bitmap) but we only want index of the first match.
I think we can leave it as follow up task to benchmark if visiting the array directly will give better performance.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#discussion_r636878524
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -225,6 +225,131 @@ Result<std::unique_ptr<KernelState>> AllInit(KernelContext*, const KernelInitArg
return ::arrow::internal::make_unique<BooleanAllImpl>();
}
+// ----------------------------------------------------------------------
+// Index implementation
+
+template <typename ArgType>
+struct IndexImpl : public ScalarAggregator {
+ using ArgValue = typename internal::GetViewType<ArgType>::T;
+
+ explicit IndexImpl(IndexOptions options, KernelState* raw_state)
+ : options(std::move(options)), seen(0), index(-1) {
+ if (auto state = static_cast<IndexImpl<ArgType>*>(raw_state)) {
+ seen = state->seen;
+ index = state->index;
+ }
+ }
+
+ Status Consume(KernelContext* ctx, const ExecBatch& batch) override {
+ // short-circuit
+ if (index >= 0 || !options.value->is_valid) {
+ return Status::OK();
+ }
+
+ auto input = batch[0].array();
+ seen = input->length;
+ const ArgValue desired = internal::UnboxScalar<ArgType>::Unbox(*options.value);
+ int64_t i = 0;
+
+ ARROW_UNUSED(internal::VisitArrayValuesInline<ArgType>(
Review comment:
Ah, nice, thanks for improving this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#issuecomment-844401662
https://issues.apache.org/jira/browse/ARROW-2665
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#discussion_r636665944
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -225,6 +225,131 @@ Result<std::unique_ptr<KernelState>> AllInit(KernelContext*, const KernelInitArg
return ::arrow::internal::make_unique<BooleanAllImpl>();
}
+// ----------------------------------------------------------------------
+// Index implementation
+
+template <typename ArgType>
+struct IndexImpl : public ScalarAggregator {
+ using ArgValue = typename internal::GetViewType<ArgType>::T;
+
+ explicit IndexImpl(IndexOptions options, KernelState* raw_state)
+ : options(std::move(options)), seen(0), index(-1) {
+ if (auto state = static_cast<IndexImpl<ArgType>*>(raw_state)) {
+ seen = state->seen;
+ index = state->index;
+ }
+ }
+
+ Status Consume(KernelContext* ctx, const ExecBatch& batch) override {
+ // short-circuit
+ if (index >= 0 || !options.value->is_valid) {
+ return Status::OK();
+ }
+
+ auto input = batch[0].array();
+ seen = input->length;
+ const ArgValue desired = internal::UnboxScalar<ArgType>::Unbox(*options.value);
+ int64_t i = 0;
+
+ ARROW_UNUSED(internal::VisitArrayValuesInline<ArgType>(
Review comment:
`VisitArrayValuesInline` handles null bitmaps in blocks and normally with better performance.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10358:
URL: https://github.com/apache/arrow/pull/10358#discussion_r636170228
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -218,6 +218,65 @@ Result<std::unique_ptr<KernelState>> AllInit(KernelContext*, const KernelInitArg
return ::arrow::internal::make_unique<BooleanAllImpl>();
}
+// ----------------------------------------------------------------------
+// Index implementation
+
+struct IndexImpl : public ScalarAggregator {
+ explicit IndexImpl(IndexOptions options, int64_t seen, int64_t index)
+ : options(std::move(options)), seen{seen}, index{index} {}
+
+ Status Consume(KernelContext* ctx, const ExecBatch& batch) override {
+ // short-circuit
+ if (index >= 0 || !options.value->is_valid) {
+ return Status::OK();
+ }
+
+ const auto& data = *batch[0].array();
+ seen = data.length;
+ ARROW_ASSIGN_OR_RAISE(
+ auto result, CallFunction("equal", {data, options.value}, ctx->exec_context()));
Review comment:
I took a second look and it ended up not being too bad to directly implement comparison, so now it doesn't rely on the equal kernel - it reuses the same infrastructure underneath so hopefully there's not too much code bloat either.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 closed pull request #10358: ARROW-2665: [C++][Python] Add index() kernel
Posted by GitBox <gi...@apache.org>.
cyb70289 closed pull request #10358:
URL: https://github.com/apache/arrow/pull/10358
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org