ARROW-10959: [C++] Add scalar string join kernel by maartenbreddels · Pull Request #8990 · apache/arrow

maartenbreddels · 2020-12-22T12:54:28Z

@jorisvandenbossche I've implemented this kernel as a binary (arity) kernel, so the input list array and the separator input string array can both be an array (see python test).

I did not implement the case where the input list is a scalar, and the separator an array, since I don't think that's very common.

And note that the kernel is named binary_join because it takes string-like and binary-like inputs.

github-actions · 2020-12-22T13:21:56Z

https://issues.apache.org/jira/browse/ARROW-10959

cpp/src/arrow/compute/kernels/scalar_string_test.cc

maartenbreddels · 2021-03-26T14:35:33Z

Failures seem unrelated.
@pitrou this is ready for review assuming @jorisvandenbossche agrees with the current behavior

pitrou

Thanks @maartenbreddels . Here are some comments.

cpp/src/arrow/compute/kernels/scalar_string.cc

cpp/src/arrow/compute/kernels/scalar_string_test.cc

cpp/src/arrow/compute/kernels/scalar_string.cc

ianmcook · 2021-03-29T19:28:48Z

Regarding "join" in the name:
+1 for consistency with Python's join function
-1 because as Arrow gains more database-like features, the word "join" is likely to confuse people. "concat" might be better.

Regarding "binary" in the name:
IMO that is likely to cause users to get the impression that this is not for strings.

It would also be good for the name to reflect that this operates on a list array.

How about renaming it list_concat?

westonpace · 2021-03-29T19:47:22Z

I'll just point out that "join" is not a python-ism. There is a string join in Java, Rust, C#, JavaScript, etc. and it is consistently called join. I think R is the only language I can find that doesn't call it "join".

ianmcook · 2021-03-29T19:48:21Z

I'll just point out that "join" is not a python-ism. There is a string join in Java, Rust, C#, JavaScript, etc. and it is consistently called join. I think R is the only language I can find that doesn't call it "join".

SQL—which probably has more users than all those languages combined 😁

ianmcook · 2021-03-29T19:55:49Z

I'll just point out that "join" is not a python-ism. There is a string join in Java, Rust, C#, JavaScript, etc. and it is consistently called join. I think R is the only language I can find that doesn't call it "join".

SQL—which probably has more users than all those languages combined 😁

Snark aside, that's a good point, and since users who are thinking in the SQL way will always be at least one abstraction layer removed from the C++ library, that makes me think "join" is fine 👍

jorisvandenbossche · 2021-03-29T19:58:29Z

In SQL it's called CONCAT? (https://www.w3schools.com/sql/func_mysql_concat.asp, although this doesn't have the concept of a join separator)

ianmcook · 2021-03-29T20:01:08Z

In SQL it's called CONCAT? (https://www.w3schools.com/sql/func_mysql_concat.asp, although this doesn't have the concept of a join separator)

In SQL, concat_ws is the variant with a separator

pitrou · 2021-05-10T15:54:32Z

@maartenbreddels Do you want to update this PR?

pitrou · 2021-06-02T16:20:05Z

I'm going to take this up.

pitrou · 2021-06-03T10:57:43Z

I adressed review comments, added a benchmark, and made binary_join 3x faster (in particular by preallocating data at the right size).

pitrou · 2021-06-03T10:57:57Z

Still a TODO to address (also support large_list inputs).

Edit: done

cpp/src/arrow/array/builder_binary.h

docs/source/cpp/compute.rst

bkietz · 2021-06-03T15:30:45Z

cpp/src/arrow/compute/kernels/test_util.h

To keep the public overloads to a minimum, these should probably just take Datums. Added https://issues.apache.org/jira/browse/ARROW-12953

cpp/src/arrow/compute/kernels/scalar_string.cc

bkietz · 2021-06-03T16:28:10Z

cpp/src/arrow/compute/kernels/scalar_string.cc

We don't lose anything for string kernels which always do their own allocation, but it's worth noting that *out is always a correctly shaped ArrayData. We could also safely write

Suggested change

std::shared_ptr<Array> string_array;

RETURN_NOT_OK(builder->Finish(&string_array));

*out = *string_array->data();

RETURN_NOT_OK(builder->FinishInternal(const_cast<std::shared_ptr<ArrayData>*>(&out->array())));

github-actions bot added Component: C++ Component: Python labels Dec 22, 2020

jorisvandenbossche reviewed Jan 4, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated Show resolved Hide resolved

maartenbreddels force-pushed the ARROW-10959 branch from 8f6e26e to 609a388 Compare January 12, 2021 18:23

maartenbreddels mentioned this pull request Jan 25, 2021

String features for use with Apache Arrow v3.0 vaexio/vaex#1186

Merged

5 tasks

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

maartenbreddels force-pushed the ARROW-10959 branch from 609a388 to d42058a Compare March 12, 2021 11:48

maartenbreddels force-pushed the ARROW-10959 branch from d42058a to 9c0836d Compare March 26, 2021 09:41

pitrou requested changes Mar 29, 2021

View reviewed changes

pitrou force-pushed the ARROW-10959 branch 2 times, most recently from f06549e to 9353e4e Compare June 3, 2021 10:56

pitrou requested a review from bkietz June 3, 2021 11:52

pitrou approved these changes Jun 3, 2021

View reviewed changes

bkietz requested changes Jun 3, 2021

View reviewed changes

bkietz approved these changes Jun 3, 2021

View reviewed changes

pitrou force-pushed the ARROW-10959 branch from dd178e6 to 489f0c8 Compare June 7, 2021 14:29

maartenbreddels and others added 2 commits June 7, 2021 16:33

ARROW-10959: [C++] Add scalar string join kernel

4458a3d

Updates and perf improvements

1ab4190

pitrou added 3 commits June 7, 2021 16:33

Add LargeList support, test AppendToCurrent

b52f8cd

Address review comments

0f80346

s/AppendToCurrent/ExtendCurrent/

27cc074

pitrou force-pushed the ARROW-10959 branch from 489f0c8 to 27cc074 Compare June 7, 2021 14:49

pitrou closed this in 2820b25 Jun 7, 2021

asfimport mentioned this pull request Jun 7, 2021

[C++] Add scalar string join kernel #26882

Closed

-    std::shared_ptr<Array> string_array;
-    RETURN_NOT_OK(builder->Finish(&string_array));
-    *out = *string_array->data();
+    RETURN_NOT_OK(builder->FinishInternal(const_cast<std::shared_ptr<ArrayData>*>(&out->array())));

Conversation

maartenbreddels commented Dec 22, 2020

Uh oh!

github-actions bot commented Dec 22, 2020

Uh oh!

Uh oh!

maartenbreddels commented Mar 26, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ianmcook commented Mar 29, 2021

Uh oh!

westonpace commented Mar 29, 2021

Uh oh!

ianmcook commented Mar 29, 2021

Uh oh!

ianmcook commented Mar 29, 2021

Uh oh!

jorisvandenbossche commented Mar 29, 2021

Uh oh!

ianmcook commented Mar 29, 2021

Uh oh!

pitrou commented May 10, 2021

Uh oh!

pitrou commented Jun 2, 2021

Uh oh!

pitrou commented Jun 3, 2021

Uh oh!

pitrou commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pitrou commented Jun 3, 2021 •

edited

Loading