maartenbreddels commented on a change in pull request #8990:
URL: https://github.com/apache/arrow/pull/8990#discussion_r602143253
##########
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:
> 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)
Yes, although you could say you'd want a drop_null to implement the `["a",
null, "b"]` -> `"a-b"` behavior. So I still think the current behaviour gives
the most flexibility (assuming the existence of a drop_null for lists)
--
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:
[email protected]
