jorisvandenbossche commented on a change in pull request #8990:
URL: https://github.com/apache/arrow/pull/8990#discussion_r601484145
##########
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:
Sorry for the late reply here. Using `fill_null` (with empty string I
suppose) would not be *fully* equivalent compared to skipping nulls, I think
(assuming `["a", null, "b"]` with separator `"-"`, the result could be `"a--b"`
vs `a-b`).
That said, I am not sure myself what the best / most useful behaviour would
be. So it's probably fine to pick whatever behaviour (not sure what's the
easiest regarding implementation?) and potentially add an option for it later
if there is demand for it (similar to how we are adding an option to the
numeric reductions for skipping nulls or not).
--
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:
us...@infra.apache.org
