ARROW-15029: [C++] Split compute/kernels/scalar_string.cc #12084
Conversation
Note: some kernels used to exist regardless of ARROW_WITH_UTF8PROC, but now only exist when ARROW_WITH_UTF8PROC is enabled. The change is not reflected in the tests, so the tests should currently fail for -DARROW_WITH_UTF8PROC=OFF.
|
|
cpp/src/arrow/CMakeLists.txt
Outdated
| @@ -422,7 +422,9 @@ if(ARROW_COMPUTE) | |||
| compute/kernels/scalar_nested.cc | |||
| compute/kernels/scalar_random.cc | |||
| compute/kernels/scalar_set_lookup.cc | |||
| compute/kernels/scalar_string.cc | |||
| compute/kernels/scalar_string_common.cc | |||
There was a problem hiding this comment.
The current convention is to suffix file names with _internal when its contents are local common utilities. Based on this, I suggest scalar_string_common.cc --> scalar_string_internal.cc.
There was a problem hiding this comment.
Okay, done in 36e134b. Based on other *_internal.h files, I guess that will also satisfy the linter w.r.t. RETURN_NOT_OK macro usage in the corresponding header file?
…names to not pollute the containing namespace too much
| namespace compute { | ||
| namespace internal { | ||
|
|
||
| FunctionDoc StringPredicateDoc(std::string summary, std::string description) { |
There was a problem hiding this comment.
This new file looks not necessary. Move these two functions to scalar_string_ascii.cc? Or to the header (static)?
There was a problem hiding this comment.
Those two functions are used by both scalar_string_ascii.cc and scalar_string_utf8.cc equally, so putting them in the header seems more suitable to me. I tried that initially by means of putting all the code in the header in an anonymous namespace, mimicking how the code used to work, but the linter and/or clang-tidy wouldn't have it. I'll give static a try.
There was a problem hiding this comment.
IMO, scalar_string_internal.cc should contain the definition of all the methods/functions in scalar_string_internal.h, instead of having them in embedded in *.h files. It makes it easier to understand the API of all those classes, but this is not followed systematically in Arrow and, if we want to do this, then it can be done in a follow-up PR.
|
@ursabot please benchmark |
|
Benchmark runs are scheduled for baseline = ec38aeb and contender = 3d77c90. Results will be available as each benchmark for each run completes. |
|
RTools CI failures are not related. |
|
Thanks @jvanstraten @edponce ! |
|
Benchmark runs are scheduled for baseline = 2d1bd96 and contender = 540dbf6. 540dbf6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This PR splits compute/kernels/scalar_string.cc into a file for ASCII/binary-based kernels and a file for UTF-8-based kernels to speed up compilation and improve readability. Kernel definitions within the files are also grouped and ordered more consistently for readability.