New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementation string concat #1720

Merged

alamb merged 7 commits into apache:master from ismailmaj:stringconcat

May 25, 2022

Contributor

ismailmaj commented May 21, 2022

Which issue does this PR close?

Closes #1540 .

Rationale for this change

What changes are included in this PR?

Add an implementation for string concat and a single test.

Are there any user-facing changes?


          first implementation string concat

9e6a0f2

github-actions bot added the arrow label

ismailmaj marked this pull request as ready for review

May 22, 2022 08:54

codecov-commenter commented May 22, 2022 •

edited

Loading

Codecov Report

Merging #1720 (e9bea71) into master (5cf06bf) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head e9bea71 differs from pull request most recent head bdad3b0. Consider uploading reports for the commit bdad3b0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1720      +/-   ##
==========================================
+ Coverage   83.32%   83.34%   +0.02%     
==========================================
  Files         196      197       +1     
  Lines       55961    56031      +70     
==========================================
+ Hits        46627    46698      +71     
+ Misses       9334     9333       -1

Impacted Files	Coverage Δ
arrow/src/compute/kernels/string.rs	`100.00% <100.00%> (ø)`
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (+0.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cf06bf...bdad3b0. Read the comment docs.

tustvold reviewed

View reviewed changes

Contributor

tustvold left a comment •

edited

Loading

I think this is a good start, thank you, I've left a few comments. Let me know if anything is unclear

arrow/src/compute/kernels/concat.rs Outdated

@@ @@ -102,6 +102,25 @@ pub fn concat(arrays: &[&dyn Array]) -> Result<ArrayRef> { @@
                   Ok(make_array(mutable.freeze()))
               }
+              // Elementwise concatenation of StringArrays
+              pub fn string_concat<Offset: OffsetSizeTrait>(

Contributor

tustvold May 22, 2022

I wonder if this should be in a new string module, as opposed to concat, as I could see it causing confusion that this performs element-wise concatenation, when the concat kernel above concatenates arrays?

Contributor

tustvold May 22, 2022

Perhaps @alamb has some thoughts?

Contributor

HaoYang670 May 22, 2022

I agree with @tustvold to create a new compute kernel. Also, list, binary, fixed size list and fix size binary can all be supported.

Contributor

alamb May 24, 2022

I also agree that a new string (or utf8 module) would be good here.

There are a bunch of other potential candidates in datafusion to move into arrow-rs (kudos to @ovr for implementing many of them initially): https://github.com/apache/arrow-datafusion/blob/master/datafusion/physical-expr/src/string_expressions.rs

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/concat.rs Outdated

+                              None
+                          }
+                      })
+                      .collect::<GenericStringArray<Offset>>())

Contributor

tustvold May 22, 2022 •

edited

Loading

I think it would be significantly faster to do something like (not at all tested)

// TODO: Handle non-zero offset in source ArrayData

if left.len() != right.len() {
    return Err(...)
}

let nulls = match (left.data().null_bitmap(), right.data.null_bitmap()) {
  (Some(left), Some(right)) = Some((left & right)?)
  (Some(left), None) => Some(left),
  (None, Some(right)) => Some(right),
  (None, None) => None,
};
let left_offsets = left.value_offsets();
let right_offsets = right.value_offsets();

let left_values = left.value_data().as_slice();
let right_values = right.value_data().as_slice();

let left_iter = left_offsets.windows(2);
let right_iter = right_offsets.windows(2);

let mut output_offsets = BufferBuilder::<Offset>::new(left_offsets.len());
let mut output_values = BufferBuilder::<u8>::new(left_data.len() + right_data.len());
output_offsets.append(0);

for (left, right) in left_iter.zip(right_iter) {
  output_values.append_slice(&left_values[left[0]..left[1]]);
  output_values.append_slice(&right_values[right[0]..right[1]]);)
  
  output_offsets.append(output_values.len()); // With checked cast
}

let mut builder = ArrayDataBuilder::new(Offset::get_data_type)
  .len(left.len())
  .add_buffer(output_offsets.finish())
  .add_buffer(output_values.finish());

if let Some(nulls) = nulls {
  builder = builder.null_bit_buffer(nulls);
}

// SAFETY - offsets valid by construction
unsafe {builder.build_unchecked()}

Contributor

HaoYang670 May 22, 2022

We could file a follow-on issue to add benchmarks.

Contributor

tfeda May 22, 2022

A couple thoughts, because I spent some time digging into this:

@alamb's example in Implementation string concat #1720 suggests that "valid_str" + None => None, where both implementations in this PR go for "valid_str" + None => "valid_str". As a user, I'd prefer the later, but I thought I'd make a note of it. If the former is chosen, then compute::util has combine_option_bitmap() which works nicely for the null handling
How do we handle cases when concatenating two arrays results in the offsets overflowing? There should probably be a test case for that.

Contributor

alamb May 24, 2022

"valid_str" + None => None

This is the standard SQL semantic and it is how most other arrow kernels I know of behave, thus I think it would be best for consistency

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved


          faster implementation

09895ef

Contributor Author

ismailmaj commented May 22, 2022

@tustvold Thank you for the review and guidance!
I've pushed something that is faster and close to your implementation.
I'm still unfamiliar with the codebase, so I'm sure there are still some mistakes.

I'll add more test cases and support for non-zero offset later.

HaoYang670 reviewed

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated

@@ @@ -102,6 +102,25 @@ pub fn concat(arrays: &[&dyn Array]) -> Result<ArrayRef> { @@
                   Ok(make_array(mutable.freeze()))
               }
+              // Elementwise concatenation of StringArrays
+              pub fn string_concat<Offset: OffsetSizeTrait>(

Contributor

HaoYang670 May 22, 2022

I agree with @tustvold to create a new compute kernel. Also, list, binary, fixed size list and fix size binary can all be supported.

arrow/src/compute/kernels/concat.rs Outdated

@@ @@ -102,6 +102,25 @@ pub fn concat(arrays: &[&dyn Array]) -> Result<ArrayRef> { @@
                   Ok(make_array(mutable.freeze()))
               }
+              // Elementwise concatenation of StringArrays
+              pub fn string_concat<Offset: OffsetSizeTrait>(

Contributor

HaoYang670 May 22, 2022

Maybe we should support concatenate arbitrary number of arrays. We could file a follow-on issue to track it.

Contributor

alamb May 25, 2022

@HaoYang670 would you like to file the follow on issues, or shall I?

Contributor

HaoYang670 May 26, 2022

I will.

arrow/src/compute/kernels/concat.rs Outdated

+                              None
+                          }
+                      })
+                      .collect::<GenericStringArray<Offset>>())

Contributor

HaoYang670 May 22, 2022

We could file a follow-on issue to add benchmarks.

Contributor

HaoYang670 commented May 22, 2022

@Ismail-Maj Really a good start! Thank you for contributing.

viirya reviewed

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

martin-g reviewed

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

viirya reviewed

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

ismailmaj commented

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

tustvold reviewed

View reviewed changes

arrow/src/compute/kernels/concat.rs Outdated Show resolved Hide resolved

Contributor

tustvold commented May 23, 2022

Looking good 😄 I think the major thing this now needs to handle is sliced inputs, and tests.


          no branching for loop and handle offsets

6bf8345

Contributor Author

ismailmaj commented May 24, 2022 •

edited

Loading

Only tests left to do (and move the code from concat.rs) I believe.
@tustvold do you think this code can be ported to handle other DataTypes?

Contributor

alamb commented May 24, 2022 •

edited

Loading

Only tests left to do (and move the code from concat.rs) I believe.

@tustvold do you think this code can be ported to handle other DataTypes?

I personally suggest completing this PR and getting it merged first, and then add other DataTypes as follow on tickets (you might find others interested in working on them as well, if you don't have time)

Ismail-Maj added 3 commits

May 24, 2022 21:53


          string module and more tests

1689cc5


          empty string test

afe79e9


          no null test and error test

b7e8bb8

ismailmaj requested review from tustvold and alamb

May 25, 2022 07:48

tustvold approved these changes

View reviewed changes

Contributor

tustvold left a comment

This looks good to me, assuming CI passes, nice work 👍

Will leave open for a bit longer in case anyone else wants to review, otherwise I'll likely merge this afternoon (BST).

Contributor

tustvold commented May 25, 2022

@Ismail-Maj could you possibly merge the latest master to bring in #1743


          Merge branch 'master' into stringconcat

bdad3b0

Contributor Author

ismailmaj commented May 25, 2022

@tustvold it should be good now.

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

Looks really nice to me -- thank you @Ismail-Maj ❤️

arrow/src/compute/kernels/string.rs

+                      )
+                      .unwrap();
+                      let expected = [None, Some("foofar"), Some("barfaz")]

Contributor

alamb May 25, 2022

👍

alamb merged commit 2ba1ef4 into apache:master

This was referenced May 26, 2022

Rename the string kernel to concatenate_elements #1747

Closed

concat_elements_utf8 should accept arbitrary number of input arrays #1748

Closed

alamb mentioned this pull request

Implement concat_elements_dyn kernel #1755

Closed

alamb mentioned this pull request

Use concat_elements_utf8 from arrow rather than custom kernel apache/datafusion#3036

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

HaoYang670 HaoYang670 left review comments

tfeda tfeda left review comments

martin-g martin-g left review comments

viirya viirya left review comments

tustvold Awaiting requested review from tustvold

alamb Awaiting requested review from alamb

Labels

arrow