Skip to content

GH-46515: [C++] Enable Multi Blocks in arrow::BinaryViewBuilder::AppendArraySlice#46655

Open
andishgar wants to merge 5 commits intoapache:mainfrom
andishgar:correct-arrow-BinaryViewBuilder-AppendArraySlice
Open

GH-46515: [C++] Enable Multi Blocks in arrow::BinaryViewBuilder::AppendArraySlice#46655
andishgar wants to merge 5 commits intoapache:mainfrom
andishgar:correct-arrow-BinaryViewBuilder-AppendArraySlice

Conversation

@andishgar
Copy link
Contributor

@andishgar andishgar commented May 31, 2025

Rationale for this change

As noted here, arrow::BinaryViewBuilder::AppendArraySlice does not support cases where the total length of elements in the input array exceeds the maximum value of int32_t.

What changes are included in this PR?

This PR updatesarrow::BinaryViewBuilder::AppendArraySliceto correctly handle {StringView, BinaryView}Array inputs where the total length of non-inlined elements exceeds the maximum value of int32_t.

Are these changes tested?

Yes, the relevant unit tests were run and passed.

Are there any user-facing changes?

No.

@pitrou
Copy link
Member

pitrou commented Jun 5, 2025

This is better than the statu quo, but I think we should be able to do even better, since BinaryView allows us to reuse the data buffers without actually copying the data for each string.

What do you think @bkietz ?

@andishgar
Copy link
Contributor Author

@pitrou I think that's a great idea. Since there's no obligation for the data to be adjacent in BinaryView, let me reimplement it accordingly. I'll inform you once it's done.

@andishgar andishgar force-pushed the correct-arrow-BinaryViewBuilder-AppendArraySlice branch from cd0a41f to f9b0928 Compare June 10, 2025 06:39
@andishgar andishgar marked this pull request as draft June 10, 2025 06:39
@andishgar andishgar force-pushed the correct-arrow-BinaryViewBuilder-AppendArraySlice branch 2 times, most recently from 6d04ab8 to 99140d7 Compare June 10, 2025 08:57
@andishgar andishgar marked this pull request as ready for review June 10, 2025 10:20
@andishgar
Copy link
Contributor Author

@pitrou I've modified the logic to create view from data buffers instead of copying them. Do you have any further suggestions for improvements?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants