lidavidm commented on a change in pull request #11233:
URL: https://github.com/apache/arrow/pull/11233#discussion_r716904798
##########
File path: cpp/src/arrow/compute/kernels/scalar_string.cc
##########
@@ -1628,12 +1627,10 @@ const FunctionDoc utf8_slice_codeunits_doc(
void AddSlice(FunctionRegistry* registry) {
auto func = std::make_shared<ScalarFunction>("utf8_slice_codeunits",
Arity::Unary(),
&utf8_slice_codeunits_doc);
- using t32 = SliceCodeunits<StringType>;
- using t64 = SliceCodeunits<LargeStringType>;
- DCHECK_OK(
- func->AddKernel({utf8()}, utf8(), t32::Exec,
SliceCodeunitsTransform::State::Init));
- DCHECK_OK(func->AddKernel({large_utf8()}, large_utf8(), t64::Exec,
- SliceCodeunitsTransform::State::Init));
+ for (const auto& ty : StringTypes()) {
+ auto exec = GenerateTypeAgnosticVarBinary<SliceCodeunits>(ty);
Review comment:
Ah, I see now. In that case, I don't think the generator really buys us
that much here. But since the change is made already it's not a big deal.
##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -1188,6 +1188,24 @@ ArrayKernelExec
GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
}
}
+// similar to GenerateTypeAgnosticPrimitive, but for variable binary and
string types
+template <template <typename...> class Generator, typename... Args>
+ArrayKernelExec GenerateTypeAgnosticVarBinary(detail::GetTypeId get_id) {
Review comment:
I'd rather continue using VarBinary since Binary would imply to me all
binary types, both variable and fixed width.
Isomorphic isn't immediately obvious to me. Maybe the Type0 variants should
be named Returning? Since Type0 is usually used as the return type. Also, both
GenerateTypeAgnosticVarBinaryBase and GenerateVarBinaryBase are type-agnostic.
That said, having both TypeAgnostic and Base is a little redundant. So maybe:
- GenerateTypeAgnosticVarBinary
- GenerateTypeAgnosticVarBinaryReturning
- GenerateVarBinaryReturning
- GenerateVarBinary
though this would be harder to implement in this PR, unfortunately.
Or we could suffix the non-type0 arguments with ToType, e.g.
GenerateVarBinaryToVarBinary, GenerateDecimalToDecimal - perhaps a little
verbose.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
