[GitHub] [arrow] pitrou commented on a change in pull request #10857: ARROW-12944: [C++] String capitalize kernel

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

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



##########
File path: cpp/src/arrow/compute/kernels/scalar_string_test.cc
##########
@@ -395,6 +395,13 @@ TYPED_TEST(TestStringKernels, AsciiLower) {
                    "[\"aaazzæÆ&\", null, \"\", \"bbb\"]");
 }
 
+TYPED_TEST(TestStringKernels, AsciiCapitalize) {
+  this->CheckUnary("ascii_capitalize", "[]", this->type(), "[]");
+  this->CheckUnary("ascii_capitalize",
+                   "[\"aAazZæÆ&\", null, \"\", \"bBB\", \"one word\"]", this->type(),
+                   "[\"AAazZæÆ&\", null, \"\", \"BBB\", \"One word\"]");

Review comment:
       Can you test a bit more thoroughly?
   For example in Python:
   ```python
   >>> "hEllo world".capitalize()
   'Hello world'
   ```

##########
File path: cpp/src/arrow/compute/kernels/scalar_string.cc
##########
@@ -443,6 +443,24 @@ struct AsciiLower {
   }
 };
 
+struct AsciiCapitalizeTransform : public StringTransformBase {
+  int64_t Transform(const uint8_t* input, int64_t input_string_ncodeunits,
+                    uint8_t* output) {
+    if (input_string_ncodeunits > 0) {
+      output[0] = ascii_toupper(input[0]);
+      std::memcpy(output + 1, input + 1, input_string_ncodeunits - 1);
+    }
+    return input_string_ncodeunits;
+  }
+
+  Status InvalidStatus() override {

Review comment:
       It's not clear to me why this function is being overriden if the function always returns successfully.

##########
File path: cpp/src/arrow/compute/kernels/scalar_string.cc
##########
@@ -4018,6 +4036,13 @@ const FunctionDoc ascii_lower_doc(
      "non-ASCII characters, use \"utf8_lower\" instead."),
     {"strings"});
 
+const FunctionDoc ascii_capitalize_doc(
+    "Capilatize the first character of ASCII input",
+    ("For each string in `strings`, return a capitalized version.\n\n"
+     "This function assumes the input is fully ASCII.  If it may contain\n"
+     "non-ASCII characters, use \"utf8_capitalize\" instead."),

Review comment:
       I don't see utf8_capitalize in this PR. Is this an oversight?




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