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 2020/11/10 16:32:13 UTC
[GitHub] [arrow] maartenbreddels opened a new pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
maartenbreddels opened a new pull request #8628:
URL: https://github.com/apache/arrow/pull/8628
The Python test fails for the `large_binary` test, with output:
```
arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
result = arr.fill_null('ccc')
expected = pa.array([b'a', b'bb', b'ccc'])
> assert result.equals(expected)
E assert False
E + where False = <built-in method equals of pyarrow.lib.LargeBinaryArray object at 0x7f762fb38520>(<pyarrow.lib.BinaryArray object at 0x7f762fb38600>\n[\n 61,\n 6262,\n 636363\n])
E + where <built-in method equals of pyarrow.lib.LargeBinaryArray object at 0x7f762fb38520> = <pyarrow.lib.LargeBinaryArray object at 0x7f762fb38520>\n[\n 61,\n 6262,\n 636363\n].equals
```
Could this be a bug in equals for LargeBinaryArray?
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522325504
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -860,6 +860,16 @@ def test_fill_null():
expected = pa.array([None, None, None, None])
assert result.equals(expected)
+ arr = pa.array(['a', 'bb', None])
+ result = arr.fill_null('ccc')
+ expected = pa.array(['a', 'bb', 'ccc'])
+ assert result.equals(expected)
+
+ arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
+ result = arr.fill_null('ccc')
Review comment:
When writing it, I was doing it as a check to the system, expecting it to fail, and forget to change this. However, thinking about it, I'm not sure how it should be checked since we have both LargeStringScalar and StringScalar.
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520769300
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -84,6 +84,52 @@ struct FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>> {
}
};
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value =
+ static_cast<util::string_view>(*fill_value_scalar.value);
Review comment:
Totally bizar, it's a copy-paste from some previous code of mine, will also change that.
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522327950
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -1058,6 +1058,22 @@ ArrayKernelExec GenerateTypeAgnosticPrimitive(detail::GetTypeId get_id) {
}
}
+/// similar to GenerateTypeAgnosticPrimitive, but for variable types
+template <template <typename...> class Generator>
+ArrayKernelExec GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
+ switch (get_id.id) {
+ case Type::BINARY:
+ case Type::STRING:
+ return Generator<BinaryType>::Exec;
Review comment:
Do you think it's overkill?
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522341768
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -860,6 +860,16 @@ def test_fill_null():
expected = pa.array([None, None, None, None])
assert result.equals(expected)
+ arr = pa.array(['a', 'bb', None])
+ result = arr.fill_null('ccc')
+ expected = pa.array(['a', 'bb', 'ccc'])
+ assert result.equals(expected)
+
+ arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
+ result = arr.fill_null('ccc')
Review comment:
Yes, it's probably ok.
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522312822
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -153,6 +153,60 @@ void AddBasicFillNullKernels(ScalarKernel kernel, ScalarFunction* func) {
AddKernels({boolean(), null()});
}
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value(*fill_value_scalar.value);
+ ArrayData* output = out->mutable_array();
+
+ // Ensure the kernel is configured properly to have no validity bitmap /
+ // null count 0 unless we explicitly propagate it below.
+ DCHECK(output->buffers[0] == nullptr);
+
+ if (input.MayHaveNulls() && fill_value_scalar.is_valid) {
Review comment:
Instead of calling `MayHaveNulls`, just compute the null count and check it here.
##########
File path: docs/source/cpp/compute.rst
##########
@@ -431,17 +431,17 @@ Structural transforms
.. XXX (this category is a bit of a hodgepodge)
-+--------------------------+------------+---------------------------------------+---------------------+---------+
-| Function name | Arity | Input types | Output type | Notes |
-+==========================+============+=======================================+=====================+=========+
-| fill_null | Binary | Boolean, Null, Numeric, Temporal | Boolean | \(1) |
-+--------------------------+------------+---------------------------------------+---------------------+---------+
-| is_null | Unary | Any | Boolean | \(2) |
-+--------------------------+------------+---------------------------------------+---------------------+---------+
-| is_valid | Unary | Any | Boolean | \(2) |
-+--------------------------+------------+---------------------------------------+---------------------+---------+
-| list_value_length | Unary | List-like | Int32 or Int64 | \(4) |
-+--------------------------+------------+---------------------------------------+---------------------+---------+
++--------------------------+------------+------------------------------------------------+---------------------+---------+
+| Function name | Arity | Input types | Output type | Notes |
++==========================+============+================================================+=====================+=========+
+| fill_null | Binary | Boolean, Null, Numeric, Temporal, String-like | Input type | \(1) |
Review comment:
Thanks :-)
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -153,6 +153,60 @@ void AddBasicFillNullKernels(ScalarKernel kernel, ScalarFunction* func) {
AddKernels({boolean(), null()});
}
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value(*fill_value_scalar.value);
+ ArrayData* output = out->mutable_array();
+
+ // Ensure the kernel is configured properly to have no validity bitmap /
+ // null count 0 unless we explicitly propagate it below.
+ DCHECK(output->buffers[0] == nullptr);
+
+ if (input.MayHaveNulls() && fill_value_scalar.is_valid) {
+ BuilderType builder(input.type, ctx->memory_pool());
+ //
Review comment:
Did you mean to say something here?
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -1058,6 +1058,22 @@ ArrayKernelExec GenerateTypeAgnosticPrimitive(detail::GetTypeId get_id) {
}
}
+/// similar to GenerateTypeAgnosticPrimitive, but for variable types
+template <template <typename...> class Generator>
+ArrayKernelExec GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
+ switch (get_id.id) {
+ case Type::BINARY:
+ case Type::STRING:
+ return Generator<BinaryType>::Exec;
Review comment:
Why don't you use `StringType` for `Type::STRING`? Fear of code duplication?
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -860,6 +860,16 @@ def test_fill_null():
expected = pa.array([None, None, None, None])
assert result.equals(expected)
+ arr = pa.array(['a', 'bb', None])
+ result = arr.fill_null('ccc')
+ expected = pa.array(['a', 'bb', 'ccc'])
+ assert result.equals(expected)
+
+ arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
+ result = arr.fill_null('ccc')
Review comment:
Hmm, we can pass a unicode string here? Interesting.
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522326005
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -1058,6 +1058,22 @@ ArrayKernelExec GenerateTypeAgnosticPrimitive(detail::GetTypeId get_id) {
}
}
+/// similar to GenerateTypeAgnosticPrimitive, but for variable types
+template <template <typename...> class Generator>
+ArrayKernelExec GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
+ switch (get_id.id) {
+ case Type::BINARY:
+ case Type::STRING:
+ return Generator<BinaryType>::Exec;
Review comment:
Yeah, following the convention I was being used in the fill_na for primitives/temporals.
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520701716
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -84,6 +84,52 @@ struct FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>> {
}
};
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value =
+ static_cast<util::string_view>(*fill_value_scalar.value);
+ ArrayData* output = out->mutable_array();
+
+ // Ensure the kernel is configured properly to have no validity bitmap /
+ // null count 0 unless we explicitly propagate it below.
+ DCHECK(output->buffers[0] == nullptr);
+
+ if (input.MayHaveNulls() && fill_value_scalar.is_valid) {
+ BuilderType builder(input.type, ctx->memory_pool());
+ //
+ KERNEL_RETURN_IF_ERROR(
+ ctx, builder.ReserveData(input.buffers[2]->size() +
+ fill_value.length() * input.GetNullCount()));
+ KERNEL_RETURN_IF_ERROR(ctx, builder.Resize(input.length + 1));
Review comment:
I accidentally forgot the +1 here, and everything worked. Does CI include valgrind, and should it pick this up?
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520725613
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -84,6 +84,52 @@ struct FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>> {
}
};
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value =
+ static_cast<util::string_view>(*fill_value_scalar.value);
Review comment:
The `static_cast` looks weird IMHO. How about using a constructor call?
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520776198
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -860,6 +860,17 @@ def test_fill_null():
expected = pa.array([None, None, None, None])
assert result.equals(expected)
+ arr = pa.array(['a', 'bb', None])
+ result = arr.fill_null('ccc')
+ expected = pa.array(['a', 'bb', 'ccc'])
+ assert result.equals(expected)
+
+ arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
+ result = arr.fill_null('ccc')
+ expected = pa.array([b'a', b'bb', b'ccc'])
Review comment:
Thanks, I stared at that piece of code for quite a bit!
----------------------------------------------------------------
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] maartenbreddels commented on pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#issuecomment-726748561
@pitrou i think this is ready to go/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8628:
URL: https://github.com/apache/arrow/pull/8628
----------------------------------------------------------------
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] maartenbreddels commented on pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#issuecomment-724881905
Looking at the implementation of `VisitArrayDataInline`, which is quite similar to what is being done in `FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>>`, I don't see much room for improvement.
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520724980
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -84,6 +84,52 @@ struct FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>> {
}
};
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value =
+ static_cast<util::string_view>(*fill_value_scalar.value);
+ ArrayData* output = out->mutable_array();
+
+ // Ensure the kernel is configured properly to have no validity bitmap /
+ // null count 0 unless we explicitly propagate it below.
+ DCHECK(output->buffers[0] == nullptr);
+
+ if (input.MayHaveNulls() && fill_value_scalar.is_valid) {
+ BuilderType builder(input.type, ctx->memory_pool());
+ //
+ KERNEL_RETURN_IF_ERROR(
+ ctx, builder.ReserveData(input.buffers[2]->size() +
+ fill_value.length() * input.GetNullCount()));
+ KERNEL_RETURN_IF_ERROR(ctx, builder.Resize(input.length + 1));
Review comment:
The +1 should not be necessary. This is the number of values appended, not the number of offsets.
----------------------------------------------------------------
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] maartenbreddels commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
maartenbreddels commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522327624
##########
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##########
@@ -153,6 +153,60 @@ void AddBasicFillNullKernels(ScalarKernel kernel, ScalarFunction* func) {
AddKernels({boolean(), null()});
}
+template <typename Type>
+struct FillNullFunctor<Type, enable_if_t<is_base_binary_type<Type>::value>> {
+ using BuilderType = typename TypeTraits<Type>::BuilderType;
+
+ static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+ const ArrayData& input = *batch[0].array();
+ const auto& fill_value_scalar =
+ checked_cast<const BaseBinaryScalar&>(*batch[1].scalar());
+ util::string_view fill_value(*fill_value_scalar.value);
+ ArrayData* output = out->mutable_array();
+
+ // Ensure the kernel is configured properly to have no validity bitmap /
+ // null count 0 unless we explicitly propagate it below.
+ DCHECK(output->buffers[0] == nullptr);
+
+ if (input.MayHaveNulls() && fill_value_scalar.is_valid) {
+ BuilderType builder(input.type, ctx->memory_pool());
+ //
Review comment:
No, forgot to commit 👍
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r520727173
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -860,6 +860,17 @@ def test_fill_null():
expected = pa.array([None, None, None, None])
assert result.equals(expected)
+ arr = pa.array(['a', 'bb', None])
+ result = arr.fill_null('ccc')
+ expected = pa.array(['a', 'bb', 'ccc'])
+ assert result.equals(expected)
+
+ arr = pa.array([b'a', b'bb', None], type=pa.large_binary())
+ result = arr.fill_null('ccc')
+ expected = pa.array([b'a', b'bb', b'ccc'])
Review comment:
You forgot to pass the type here.
----------------------------------------------------------------
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] pitrou commented on a change in pull request #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#discussion_r522341491
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -1058,6 +1058,22 @@ ArrayKernelExec GenerateTypeAgnosticPrimitive(detail::GetTypeId get_id) {
}
}
+/// similar to GenerateTypeAgnosticPrimitive, but for variable types
+template <template <typename...> class Generator>
+ArrayKernelExec GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
+ switch (get_id.id) {
+ case Type::BINARY:
+ case Type::STRING:
+ return Generator<BinaryType>::Exec;
Review comment:
No, it's ok. However, please don't use triple-slashes above for regular comments.
----------------------------------------------------------------
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 #8628: ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8628:
URL: https://github.com/apache/arrow/pull/8628#issuecomment-724819642
https://issues.apache.org/jira/browse/ARROW-9489
----------------------------------------------------------------
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