Add `substring` support for `FixedSizeBinaryArray` #1633

HaoYang670 · 2022-05-01T03:38:43Z

Which issue does this PR close?

Closes #1618.

What changes are included in this PR?

make the substring kernel support FixedSizeBinaryArray
add more test cases for binary and utf8
add benchmark for FixedSizeBinaryArray

Are there any user-facing changes?

Rename some benchmarks.

Bench Result

substring utf8 (start = 0, length = None)                                                                            
                        time:   [19.811 ms 19.843 ms 19.883 ms]

substring utf8 (start = 1, length = str_len - 1)                                                                            
                        time:   [21.270 ms 21.284 ms 21.299 ms]


substring fixed size binary array                                                                            
                        time:   [19.717 ms 19.753 ms 19.798 ms]

Signed-off-by: remzi <[email protected]>

viirya · 2022-05-02T06:12:40Z

arrow/src/compute/kernels/substring.rs

+    let new_len = match length {
+        Some(len) => len.min(old_len - new_start),
+        None => old_len - new_start,
+    };


Is it possible that if length is negative? DataType::FixedSizeBinary(negative) seems invalid.

I think the answer is no for several reasons:

the definition of the public function makes sure that length is always >=0

pub fn substring(array: &dyn Array, start: i64, length: Option<u64>) -> Result<ArrayRef> {

the definition of the data type of FixedSizeBinary seems to allow negative value, although it is somewhat weird:

/// Opaque binary data of fixed size. /// Enum parameter specifies the number of bytes per value. FixedSizeBinary(i32),

Although it may be more reasonable to use unsigned int to type the length, in Apache Arrow specification, the length must be i32. https://arrow.apache.org/docs/format/Columnar.html#fixed-size-list-layout (For nested arrays, we always use signed integer to represent the length or offsets.)

A fixed size list type is specified like FixedSizeList<T>[N], where T is any type (primitive or nested) and N is a 32-bit signed integer representing the length of the lists.

What would happen if we give a negative length or negative offsets buffer?
This is a fun game!

Is there any discussion or explanation of why we use signed integer to represent length?

Ah, that's right, I didn't notice that length is u64.

Since unsigned integers can be more difficult to work with in some cases (e.g. in the JVM), we recommend preferring signed integers over unsigned integers for representing dictionary indices. Additionally, we recommend avoiding using 64-bit unsigned integer indices unless they are required by an application.

This is some wordings I read in the spec. It is not talking about array length, but I think it might be somehow related to the question.

viirya

Looks good to me in general. Thanks @HaoYang670.

viirya · 2022-05-02T06:20:23Z

arrow/src/compute/kernels/substring.rs

+            // all-nulls array is always identical
+            (vec![None, None, None], -1, Some(1), vec![None, None, None]),


Thanks for adding test coverage.

viirya · 2022-05-03T01:25:06Z

arrow/src/compute/kernels/substring.rs

+            array.data_ref().null_buffer().cloned(),
+            0,


0 offset works for the new array value new_values. But null_buffer is cloned, so maybe it has different offset?

Thank you very much for finding the bug. I have fixed it by getting the bit_slice of null_buffer. Also a corresponding test is added.

As for other arrays' implementations we have the same bug, I will file a follow-up issue to fix it.

Thank you @HaoYang670

Signed-off-by: remzi <[email protected]>

codecov-commenter · 2022-05-03T12:24:59Z

Codecov Report

Merging #1633 (566e0ff) into master (7d00e3c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   83.02%   83.08%   +0.05%     
==========================================
  Files         193      193              
  Lines       55577    55772     +195     
==========================================
+ Hits        46145    46338     +193     
- Misses       9432     9434       +2

Impacted Files	Coverage Δ
arrow/src/compute/kernels/substring.rs	`100.00% <100.00%> (ø)`
arrow/src/array/transform/mod.rs	`86.57% <0.00%> (-0.23%)`	⬇️
arrow/src/array/array_union.rs	`90.63% <0.00%> (-0.09%)`	⬇️
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (ø)`
parquet/src/arrow/arrow_writer.rs	`97.66% <0.00%> (+<0.01%)`	⬆️
parquet/src/encodings/encoding.rs	`93.56% <0.00%> (+0.18%)`	⬆️
arrow/src/array/array_binary.rs	`93.26% <0.00%> (+0.28%)`	⬆️

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 7d00e3c...566e0ff. Read the comment docs.

viirya · 2022-05-04T20:58:00Z

I will merge this later today if no more comments.

cc @tustvold @alamb

alamb

I don't have any more comments other than thank you @HaoYang670 and @viirya 👍

viirya · 2022-05-05T01:30:22Z

Thank you @HaoYang670 @alamb ! 😄

HaoYang670 · 2022-05-05T01:32:08Z

Thanks a lot for your review @viirya @alamb !

HaoYang670 added 7 commits May 1, 2022 09:46

add function, no tests yet

bc1484d

Signed-off-by: remzi <[email protected]>

adjust the fn structure

4065ae6

Signed-off-by: remzi <[email protected]>

cargo fmt

a22e7cc

Signed-off-by: remzi <[email protected]>

add tests

3373b6c

Signed-off-by: remzi <[email protected]>

fix clippy

e73a737

Signed-off-by: remzi <[email protected]>

add identical test cases for utf8 and binary

c7e3624

Signed-off-by: remzi <[email protected]>

add benchmark

08bf094

Signed-off-by: remzi <[email protected]>

github-actions bot added the arrow Changes to the arrow crate label May 1, 2022

HaoYang670 mentioned this pull request May 1, 2022

Epic: enhance the substring kernel #1531

Open

12 tasks

viirya reviewed May 2, 2022

View reviewed changes

viirya reviewed May 3, 2022

View reviewed changes

fix offset bug

6906fc0

Signed-off-by: remzi <[email protected]>

HaoYang670 mentioned this pull request May 3, 2022

Bug (substring kernel): The null buffer is not aligned when offset != 0 #1639

Closed

fix a nit

566e0ff

Signed-off-by: remzi <[email protected]>

viirya approved these changes May 3, 2022

View reviewed changes

alamb reviewed May 5, 2022

View reviewed changes

viirya merged commit e51de5e into apache:master May 5, 2022

HaoYang670 deleted the fixed_size_binary_substring branch May 5, 2022 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `substring` support for `FixedSizeBinaryArray` #1633

Add `substring` support for `FixedSizeBinaryArray` #1633

HaoYang670 commented May 1, 2022 •

edited

Loading

viirya May 2, 2022

HaoYang670 May 2, 2022

HaoYang670 May 2, 2022

viirya May 3, 2022

viirya May 3, 2022

viirya left a comment •

edited

Loading

viirya May 2, 2022

viirya May 3, 2022

HaoYang670 May 3, 2022

HaoYang670 May 3, 2022

viirya May 3, 2022

codecov-commenter commented May 3, 2022

viirya commented May 4, 2022

alamb left a comment

viirya commented May 5, 2022

HaoYang670 commented May 5, 2022

		// all-nulls array is always identical
		(vec![None, None, None], -1, Some(1), vec![None, None, None]),

Add substring support for FixedSizeBinaryArray #1633

Add substring support for FixedSizeBinaryArray #1633

Conversation

HaoYang670 commented May 1, 2022 • edited Loading

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

Bench Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 3, 2022

Codecov Report

viirya commented May 4, 2022

alamb left a comment

Choose a reason for hiding this comment

viirya commented May 5, 2022

HaoYang670 commented May 5, 2022

Add `substring` support for `FixedSizeBinaryArray` #1633

Add `substring` support for `FixedSizeBinaryArray` #1633

HaoYang670 commented May 1, 2022 •

edited

Loading

viirya left a comment •

edited

Loading