feat: initial support string_view and binary_view, supports layout and basic construction + tests #5481

ariesdevil · 2024-03-07T10:10:53Z

Which issue does this PR close?

Rationale for this change

Initially support StringViewArray and BinaryViewArray, mainly for adding layout and basic construction and tests for these two new types of array.

Note: This implementation is primarily based on these two PRs [#4585 databendlabs/databend/pull/14662]

What changes are included in this PR?

Add two new types of arrays.

Are there any user-facing changes?

Yes

arrow-array/src/array/bytes_view_array.rs

tustvold

This is looking good, made some suggestions.

I also wonder if this should be ByteView and not BytesView for consistency with things like GenericByteArray, although I do agree in hindsight it probably should be GenericBytesArray

FWIW I rebased #4585 in https://github.com/tustvold/arrow-rs/tree/array-view and compared the changes this makes on top to speed up my review.

arrow-data/Cargo.toml

arrow-array/src/types.rs

arrow-data/src/data.rs

arrow-data/src/equal/bytes_view.rs

alamb

Thank you so much @ariesdevil -- I think this code is looking basically ready to me. I have some API suggestions that might be worth considering but we can do it as a follow on PR.

The only thing missing from this PR are a few more tests and then it will be ready to go

cc @XiangpengHao

arrow-array/src/array/byte_view_array.rs

alamb · 2024-03-12T11:05:21Z

arrow-array/src/array/byte_view_array.rs

+
+    /// Returns the views buffer
+    #[inline]
+    pub fn views(&self) -> &ScalarBuffer<u128> {


Not for this PR, but I wonder if we should consider implementing a ByteViewBuffer type, similarly to the OffsetBuffer used for GenericBinaryView -- https://docs.rs/arrow/latest/arrow/buffer/struct.OffsetBuffer.html

I thought that the introduction of OffsetBuffer made working with StringArray/BinaryArray much easier.

I can imagine ByteViewBuffer encapsulating the 12 byte inline string calculation, as well as building such values up as well as hosting documentation explaining what types are present.

If this seems like a reasonable idea, I can write up a ticket / maybe whack up a PR to show what it might look like

That's a good idea, I'll do it in the next PR.

alamb · 2024-03-12T11:07:31Z

arrow-array/src/builder/generic_bytes_view_builder.rs

+    pub fn append_value(&mut self, value: impl AsRef<T::Native>) {
+        let v: &[u8] = value.as_ref().as_ref();
+        let length: u32 = v.len().try_into().unwrap();
+        if length <= 12 {


With a views builder maybe this could look like

self.views_builder.append_inline(v, length)

arrow-array/src/array/byte_view_array.rs

arrow-array/src/record_batch.rs

arrow-data/src/byte_view.rs

arrow-array/src/builder/generic_bytes_view_builder.rs

arrow/tests/array_transform.rs

arrow/tests/array_equal.rs

ariesdevil · 2024-03-12T13:37:42Z

Hi @alamb , I added more tests for your kindly comments, PTAL again.

arrow-data/src/equal/byte_view.rs

arrow-data/src/byte_view.rs

alamb

Thank you again @ariesdevil -- I think a few more tests and some cleanups and this PR will be good to go from my perspective

arrow-data/src/equal/byte_view.rs

arrow-array/src/builder/generic_bytes_view_builder.rs

arrow-array/src/array/byte_view_array.rs

arrow-data/src/byte_view.rs

arrow-array/src/array/byte_view_array.rs

arrow-array/src/builder/generic_bytes_view_builder.rs

ariesdevil · 2024-03-13T03:15:23Z

Hi @alamb @tustvold , I modified the code as you guys suggested and added more tests, PTAL again.

alamb

Thank you @ariesdevil -- I think this PR is ready to go from my perspective.

All remaining issues / comments I think we can address as follow on PRs.

I plan to merge this tomorrow morning (around 18 hours from now) unless anyone else would like additional time to review.

Thanks again @ariesdevil

…s padded with `0` (#40512) ### Rationale for this change While implementing `Variable-size Binary View Layout` (thanks @ ariesdevil !) in apache/arrow-rs#5481 it was not 100% clear if the inlined string was zero padded. @ bkietz noted that > The spec does say "padded with zero" https://github.com/apache/arrow/blob/main/docs/source/format/Columnar.rst?plain=1#L384 but it could be repeated in the surrounding paragraph. In any case, padded with zero is definitely the intent ``` * Short strings, length <= 12 | Bytes 0-3 | Bytes 4-15 | |------------|---------------------------------------| | length | data (padded with 0) | ``` ### What changes are included in this PR? Add a sentence in the surrounding text to make it clear the inlined strings values are zero padded Note I do not think this is a specification change (and therefore doesn't need a vote on the mailing list) as the spec already specifies the padding is zero (in the diagram). This simply clarifies the text to emphasize this point for ease of understanding ### Are these changes tested? ### Are there any user-facing changes? Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

alamb · 2024-03-14T18:34:06Z

🚀 I filed a bunch of follow on tickets:
IPC format support for StringViewArray and BinaryViewArray #5506
#5507
#5508
#5509
#5510
#5511
#5513

dharanad · 2024-08-14T14:31:51Z

arrow/tests/array_transform.rs

@@ -1027,6 +1028,44 @@ fn test_extend_nulls_panic() {
    mutable.extend_nulls(2);
 }

+#[test]
+fn test_string_view() {


github-actions bot added the arrow Changes to the arrow crate label Mar 7, 2024

amunra reviewed Mar 7, 2024

View reviewed changes

arrow-array/src/array/bytes_view_array.rs Outdated Show resolved Hide resolved

This was referenced Mar 9, 2024

DataFusion weekly project plan (Andrew Lamb) - March 4, 2024 apache/datafusion#9453

Closed

DataFusion weekly project plan (Andrew Lamb) - March 11, 2024 apache/datafusion#9555

Closed

ariesdevil force-pushed the string_view branch from 64ecee7 to fc6e844 Compare March 11, 2024 13:32

ariesdevil marked this pull request as ready for review March 11, 2024 13:32

ariesdevil force-pushed the string_view branch 2 times, most recently from 7e11917 to a6410ce Compare March 11, 2024 13:39

tustvold reviewed Mar 11, 2024

View reviewed changes

arrow-data/Cargo.toml Outdated Show resolved Hide resolved

arrow-array/src/types.rs Outdated Show resolved Hide resolved

arrow-data/src/data.rs Outdated Show resolved Hide resolved

arrow-data/src/equal/bytes_view.rs Outdated Show resolved Hide resolved

ariesdevil force-pushed the string_view branch 2 times, most recently from 49dc2a1 to 1fd109b Compare March 12, 2024 09:54

alamb reviewed Mar 12, 2024

View reviewed changes

ariesdevil force-pushed the string_view branch from 1fd109b to 5bda2ae Compare March 12, 2024 13:35

XiangpengHao reviewed Mar 12, 2024

View reviewed changes

arrow-data/src/equal/byte_view.rs Show resolved Hide resolved

thinkharderdev reviewed Mar 12, 2024

View reviewed changes

arrow-data/src/byte_view.rs Outdated Show resolved Hide resolved

alamb reviewed Mar 12, 2024

View reviewed changes

arrow-array/src/array/byte_view_array.rs Show resolved Hide resolved

tustvold reviewed Mar 12, 2024

View reviewed changes

arrow-array/src/builder/generic_bytes_view_builder.rs Outdated Show resolved Hide resolved

tustvold reviewed Mar 13, 2024

View reviewed changes

arrow-array/src/builder/generic_bytes_view_builder.rs Outdated Show resolved Hide resolved

ariesdevil added 2 commits March 13, 2024 11:12

support string_view and binary_view

08e13f6

fix reviewer comments

dd5aea4

ariesdevil force-pushed the string_view branch from 5bda2ae to dd5aea4 Compare March 13, 2024 03:13

ariesdevil mentioned this pull request Mar 13, 2024

chore: update arrow-format #5502

Merged

alamb approved these changes Mar 13, 2024

View reviewed changes

This was referenced Mar 13, 2024

MINOR: [Docs] Clarify inlined strings in VariableLengthStringView is padded with 0 apache/arrow#40512

Merged

[EPIC] Implement StringViewArray and BinaryViewArray #5374

Closed

alamb merged commit d39cf28 into apache:master Mar 14, 2024
25 checks passed

ariesdevil deleted the string_view branch March 15, 2024 01:58

tustvold mentioned this pull request Mar 15, 2024

Add StringViewArray implementation and layout and basic construction + tests #5469

Closed

alamb mentioned this pull request Mar 18, 2024

Implement StringViewArray and BinaryViewArray reading/writing in parquet #5530

Closed

tustvold mentioned this pull request Apr 17, 2024

Arrow Flight format support for StringViewArray and BinaryViewArray #5507

Closed

dharanad reviewed Aug 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial support string_view and binary_view, supports layout and basic construction + tests #5481

feat: initial support string_view and binary_view, supports layout and basic construction + tests #5481

ariesdevil commented Mar 7, 2024 •

edited

Loading

tustvold left a comment

alamb left a comment

alamb Mar 12, 2024

ariesdevil Mar 12, 2024

alamb Mar 12, 2024

ariesdevil Mar 12, 2024

ariesdevil commented Mar 12, 2024

alamb left a comment

ariesdevil commented Mar 13, 2024

alamb left a comment

alamb commented Mar 14, 2024

dharanad Aug 14, 2024

feat: initial support string_view and binary_view, supports layout and basic construction + tests #5481

feat: initial support string_view and binary_view, supports layout and basic construction + tests #5481

Conversation

ariesdevil commented Mar 7, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 12, 2024

Choose a reason for hiding this comment

ariesdevil Mar 12, 2024

Choose a reason for hiding this comment

alamb Mar 12, 2024

Choose a reason for hiding this comment

ariesdevil Mar 12, 2024

Choose a reason for hiding this comment

ariesdevil commented Mar 12, 2024

alamb left a comment

Choose a reason for hiding this comment

ariesdevil commented Mar 13, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Mar 14, 2024

dharanad Aug 14, 2024

Choose a reason for hiding this comment

ariesdevil commented Mar 7, 2024 •

edited

Loading