[GitHub] [arrow] pitrou commented on a change in pull request #9786: ARROW-11693: [C++] Add string length kernel

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #9786:
URL: https://github.com/apache/arrow/pull/9786#discussion_r600431620



##########
File path: cpp/src/arrow/array/array_binary_test.cc
##########
@@ -301,14 +301,18 @@ class TestStringArray : public ::testing::Test {
     auto st2 = ValidateFull(1, {0, 4}, "\xf4\x90\x80\x80");
     // Single UTF8 character straddles two entries
     auto st3 = ValidateFull(2, {0, 1, 2}, "\xc3\xa9");
+    // Null characters in the string
+    auto st4 = ValidateFull(1, {0, 4}, "\0\0\0\0");

Review comment:
       Can you explain why this is invalid? Unicode character 0 is a valid unicode character.

##########
File path: cpp/src/arrow/array/array_binary_test.cc
##########
@@ -301,14 +301,18 @@ class TestStringArray : public ::testing::Test {
     auto st2 = ValidateFull(1, {0, 4}, "\xf4\x90\x80\x80");
     // Single UTF8 character straddles two entries
     auto st3 = ValidateFull(2, {0, 1, 2}, "\xc3\xa9");
+    // Null characters in the string
+    auto st4 = ValidateFull(1, {0, 4}, "\0\0\0\0");

Review comment:
       Also, it works using PyArrow:
   ```python
   >>> arr = pa.array(["\0"])
   >>> arr
   <pyarrow.lib.StringArray object at 0x7f6befb354b0>
   [
     ""
   ]
   >>> arr.to_pylist()
   ['\x00']
   >>> arr.validate(full=True)
   >>> 
   ```

##########
File path: cpp/src/arrow/compute/kernels/scalar_string.cc
##########
@@ -66,6 +66,25 @@ struct BinaryLength {
 
 #ifdef ARROW_WITH_UTF8PROC
 
+struct Utf8Length {
+  template <typename OutValue, typename Arg0Value = util::string_view>
+  static OutValue Call(KernelContext*, Arg0Value val) {
+    auto str = reinterpret_cast<const utf8proc_uint8_t*>(val.data());
+    auto strlen = static_cast<utf8proc_ssize_t>(val.size());
+    utf8proc_int32_t codepoint;
+
+    OutValue length = 0;
+    while (strlen > 0) {
+      auto char_width = utf8proc_iterate(str, strlen, &codepoint);

Review comment:
       You may want to add a benchmark for this kernel in `scalar_string_benchmark.cc`. Then you can compare the utf8proc-based implementation against a more optimized implementation that will simply count of number of bytes smaller than 128.




-- 
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