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/06/18 01:00:03 UTC
[GitHub] [arrow] wesm opened a new pull request, #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
wesm opened a new pull request, #13398:
URL: https://github.com/apache/arrow/pull/13398
This is mostly mechanical refactoring. Since many VectorKernels support being passed in a ChunkedArray, I separated the `ExecSpan` code path (which does not support chunked arrays) from a separate `VectorKernel::exec_chunked` function which continues to use the `const ExecBatch&` input format.
Aggregate (scalar and hash) kernels will have to get refactored in a follow up PR. I was going to try to pack that into this branch but it got to be a little bit too much. I would like to work on ARROW-16757 next anyway which will help a lot with cleaning up a lot of accumulated cruft
--
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 #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13398:
URL: https://github.com/apache/arrow/pull/13398#discussion_r902604395
##########
cpp/src/arrow/array/data.cc:
##########
@@ -208,7 +215,6 @@ int64_t ArraySpan::GetNullCount() const {
int GetNumBuffers(const DataType& type) {
switch (type.id()) {
case Type::NA:
- return 0;
Review Comment:
Is this correct?
##########
cpp/src/arrow/array/data.cc:
##########
@@ -151,6 +151,13 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+ Type::type type_id = this->type->id();
+ if (data.buffers[0] == nullptr && type_id != Type::NA &&
+ type_id != Type::SPARSE_UNION && type_id != Type::DENSE_UNION) {
Review Comment:
nit: there's [`arrow::internal::HasValidityBitmap` in `arrow/type.h`](https://github.com/apache/arrow/blob/d49d8de3a9158f66ff7073fe077c5c292e38de6e/cpp/src/arrow/type.h#L2043-L2052)
##########
cpp/src/arrow/compute/kernels/vector_replace.cc:
##########
@@ -442,25 +382,91 @@ struct ReplaceWithMaskFunctor {
}
};
+template <typename Type>
+struct ReplaceMaskChunked {
+ static Status Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const Datum& mask = batch[1];
+ const Datum& replacements = batch[2];
+
+ // TODO(wesm): these assertions that the arguments cannot be ChunkedArray
+ // should happen someplace more generic, not here
Review Comment:
I think for this function we eventually do want to support chunked arrays in all positions, it just wasn't implemented
--
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 #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13398:
URL: https://github.com/apache/arrow/pull/13398#issuecomment-1159325954
https://issues.apache.org/jira/browse/ARROW-16824
--
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] pitrou commented on a diff in pull request #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13398:
URL: https://github.com/apache/arrow/pull/13398#discussion_r908502102
##########
cpp/src/arrow/array/data.cc:
##########
@@ -208,7 +215,6 @@ int64_t ArraySpan::GetNullCount() const {
int GetNumBuffers(const DataType& type) {
switch (type.id()) {
case Type::NA:
- return 0;
Review Comment:
I think that was to avoid breaking compatibility. We noticed too late that this was inconsistent with the format spec.
We bridge for the inconsistency in the IPC and C data layers.
--
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] wesm commented on pull request #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13398:
URL: https://github.com/apache/arrow/pull/13398#issuecomment-1159467765
CI is passing — this Windows thread pool failure is weird though (cc @westonpace). I’m going to start working on the next stage of work but I’ll just stack the work on this PR in a branch while this PR is in review
--
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] wesm commented on a diff in pull request #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13398:
URL: https://github.com/apache/arrow/pull/13398#discussion_r902654504
##########
cpp/src/arrow/array/data.cc:
##########
@@ -208,7 +215,6 @@ int64_t ArraySpan::GetNullCount() const {
int GetNumBuffers(const DataType& type) {
switch (type.id()) {
case Type::NA:
- return 0;
Review Comment:
Yes — if you look at `NullType::layout()` there's a single buffer that's always null, but this seems to conflict with the columnar format documentation. @pitrou do you know more about 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13398:
URL: https://github.com/apache/arrow/pull/13398#discussion_r900630977
##########
cpp/src/arrow/compute/row/grouper.cc:
##########
@@ -119,7 +125,13 @@ struct GrouperImpl : Grouper {
}
for (int i = 0; i < batch.num_values(); ++i) {
- RETURN_NOT_OK(encoders_[i]->Encode(batch[i], batch.length, key_buf_ptrs.data()));
+ ExecValue value;
+ if (batch[i].is_array()) {
+ value.SetArray(*batch[i].array());
+ } else {
+ value.SetScalar(batch[i].scalar().get());
+ }
+ RETURN_NOT_OK(encoders_[i]->Encode(value, batch.length, key_buf_ptrs.data()));
Review Comment:
This is temporary because Grouper and things hashing related are going to switch soon to use ExecSpan, so this will not be necessary once that's done
--
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 #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13398:
URL: https://github.com/apache/arrow/pull/13398#discussion_r902657898
##########
cpp/src/arrow/array/data.cc:
##########
@@ -208,7 +215,6 @@ int64_t ArraySpan::GetNullCount() const {
int GetNumBuffers(const DataType& type) {
switch (type.id()) {
case Type::NA:
- return 0;
Review Comment:
Hmm, I suppose it's like how here we have unions listed with three buffers even though the spec lists only one/two.
--
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] wesm merged pull request #13398: ARROW-16824: [C++] Migrate VectorKernels to use ExecSpan, split out ChunkedArray execution
Posted by GitBox <gi...@apache.org>.
wesm merged PR #13398:
URL: https://github.com/apache/arrow/pull/13398
--
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