[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #8990: ARROW-10959: [C++] Add scalar string join kernel

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

jorisvandenbossche commented on a change in pull request #8990:
URL: https://github.com/apache/arrow/pull/8990#discussion_r551292722



##########
File path: cpp/src/arrow/compute/kernels/scalar_string_test.cc
##########
@@ -428,6 +433,26 @@ TYPED_TEST(TestStringKernels, StrptimeDoesNotProvideDefaultOptions) {
   ASSERT_RAISES(Invalid, CallFunction("strptime", {input}));
 }
 
+TYPED_TEST(TestStringKernels, Join) {
+  auto separator = this->scalar("--");
+  CheckScalarBinary(
+      "binary_join", list(this->type()),
+      R"([["a", "bb", "ccc"], [], null, ["dd"], ["eee", null], ["ff", ""]])", separator,
+      this->type(), R"(["a--bb--ccc", "", null, "dd", null, "ff--"])");

Review comment:
       Another option could also be to skip nulls from the list, instead of skipping the list entirely if there is a null present? 
   (so eg `["eee", null]` could give `"eee"` instead of ``null``? 
   
   (don't really know what is most sensible to do, but generally for aggregations (and you can see a single join as a kind of aggregation?) nulls get skipped instead of propagated)




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