GH-42247: [C++] Support casting to and from utf8_view/binary_view #43302

felipecrv · 2024-07-17T17:06:16Z

Rationale for this change

We need casts between string (binary) and string-view (binary-view) types since they are semantically equivalent.

What changes are included in this PR?

Add is_binary_view_like() type predicate
Add BinaryViewTypes() list including STRING_VIEW/BINARY_VIEW
New cast kernels

Are these changes tested?

Yes, but test coverage might be improved.

Are there any user-facing changes?

More casts are available.

GitHub Issue: [C++] Support casting to and from utf8_view/binary_view #42247

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

pitrou · 2024-07-22T09:44:32Z

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

Is this a TODO for this PR? Otherwise, perhaps create a GH issue for it.

Fixing it now.

@mapleFU this one needs to be fixed with the same fix I added in line 477 // Check against offset overflow. I forgot that there were two places with this TODO.

cpp/src/arrow/type_traits.h

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

mapleFU

Great work! memset to 0 really handle some tricky problem in protocol layer, thanks for your effort!

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

bkietz

Looking good, just a few nits

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

cpp/src/arrow/type_traits.h

cpp/src/arrow/visit_data_inline.h

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

mapleFU · 2024-08-15T11:14:18Z

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

(Unrelated to this pr: What reminds me is the utf8 checking in arrow-rs, maybe we can use same algorithm? apache/arrow-rs#6009 )

That sounds like a good [Parquet] issue to open

Oh, I think I see what you mean: we could similarly assemble larger contiguous byte ranges on which we run a single Utf8 validation pass.

For the common case of views whose out-of-line data directly follows the previous out-of-line bytes, this would yield one long byte range for Utf8 validation.

Inline strings would also always be valid Utf8 since their size would consist of 3 zero bytes and one small byte plus the inline data and padding zero bytes, so we could validate on runs of inline views too.

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

…ecking loop

pitrou · 2024-09-12T15:58:52Z

@github-actions crossbow submit -g cpp

github-actions · 2024-09-12T16:01:37Z

Revision: 2416d19

Submitted crossbow builds: ursacomputing/crossbow @ actions-aaf4a45dfc

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14

pitrou · 2024-09-12T16:39:28Z

CI failures are unrelated, I'll merge

mapleFU · 2024-09-13T02:33:08Z

Thanks!

conbench-apache-arrow · 2024-09-13T05:36:36Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 85fc3eb.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 97 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ew (apache#43302) ### Rationale for this change We need casts between string (binary) and string-view (binary-view) types since they are semantically equivalent. ### What changes are included in this PR? - Add `is_binary_view_like()` type predicate - Add `BinaryViewTypes()` list including `STRING_VIEW/BINARY_VIEW` - New cast kernels ### Are these changes tested? Yes, but test coverage might be improved. ### Are there any user-facing changes? More casts are available. * GitHub Issue: apache#42247 Lead-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Co-authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

…tring and binary types (#44822) ### Rationale for this change Use `CopyBitmap` to optimize performance in string casting from string-view to offset string. ### What changes are included in this PR? Originally, the way we create the bitmap is by appending one bit at a time, which is slow. Since casting should not change the values in bitmap, this feature takes advantage of `CopyBitmap` to create the entire bitmap at once. Then, to create offsets and buffer array, I use `TypedBufferBuilder` as suggested in the original comment #43302 (comment). ### Are these changes tested? The original unit tests have passed. ### Are there any user-facing changes? No, the casting behavior should remain unchanged. closes [ #43573 ](#43573) * GitHub Issue: #43573 Lead-authored-by: Crystal Zhou <crystal.zhouxiaoyue@hotmail.com> Co-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Crystal Zhou <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Crystal <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>

…fset string and binary types (apache#44822) ### Rationale for this change Use `CopyBitmap` to optimize performance in string casting from string-view to offset string. ### What changes are included in this PR? Originally, the way we create the bitmap is by appending one bit at a time, which is slow. Since casting should not change the values in bitmap, this feature takes advantage of `CopyBitmap` to create the entire bitmap at once. Then, to create offsets and buffer array, I use `TypedBufferBuilder` as suggested in the original comment apache#43302 (comment). ### Are these changes tested? The original unit tests have passed. ### Are there any user-facing changes? No, the casting behavior should remain unchanged. closes [ apache#43573 ](apache#43573) * GitHub Issue: apache#43573 Lead-authored-by: Crystal Zhou <crystal.zhouxiaoyue@hotmail.com> Co-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Crystal Zhou <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Crystal <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>

…tring and binary types (#44822) ### Rationale for this change Use `CopyBitmap` to optimize performance in string casting from string-view to offset string. ### What changes are included in this PR? Originally, the way we create the bitmap is by appending one bit at a time, which is slow. Since casting should not change the values in bitmap, this feature takes advantage of `CopyBitmap` to create the entire bitmap at once. Then, to create offsets and buffer array, I use `TypedBufferBuilder` as suggested in the original comment apache/arrow#43302 (comment). ### Are these changes tested? The original unit tests have passed. ### Are there any user-facing changes? No, the casting behavior should remain unchanged. closes [ #43573 ](apache/arrow#43573) * GitHub Issue: #43573 Lead-authored-by: Crystal Zhou <crystal.zhouxiaoyue@hotmail.com> Co-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Crystal Zhou <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Crystal <45134936+CrystalZhou0529@users.noreply.github.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Jul 17, 2024

felipecrv changed the title ~~GH-43010: [C++] Support casting to and from utf8_view/binary_view~~ GH-42247: [C++] Support casting to and from utf8_view/binary_view Jul 17, 2024

This was referenced Jul 17, 2024

[C++] Support casting to and from utf8_view/binary_view #42247

Closed

[C++][Parquet] Supports write BinaryView/StringView to Parquet file #43244

Closed

felipecrv force-pushed the str2str_casts branch from a5181b5 to 7d5a377 Compare July 21, 2024 17:47

felipecrv marked this pull request as ready for review July 21, 2024 17:48

felipecrv requested a review from bkietz July 21, 2024 18:47

pitrou reviewed Jul 22, 2024

View reviewed changes

mapleFU approved these changes Jul 22, 2024

View reviewed changes

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

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

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 23, 2024

bkietz requested changes Jul 26, 2024

View reviewed changes

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

cpp/src/arrow/type_traits.h Outdated Show resolved Hide resolved

cpp/src/arrow/visit_data_inline.h Outdated Show resolved Hide resolved

felipecrv force-pushed the str2str_casts branch from 7d5a377 to 7d1f438 Compare August 5, 2024 21:27

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 5, 2024

felipecrv mentioned this pull request Aug 5, 2024

[C++] Copy bitmap all at once when casting from string-view to offset string and binary types #43573

Closed

felipecrv requested a review from pitrou August 6, 2024 00:10

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 6, 2024

pitrou reviewed Aug 6, 2024

View reviewed changes

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

mapleFU reviewed Aug 15, 2024

View reviewed changes

felipecrv and others added 13 commits September 12, 2024 17:44

scalar_cast_numeric.cc: Cast (utf8|binary)_view to numeric types

7d330d3

scalar_cast_test.cc: Include string and binary-views types in cast-ch…

d59e146

…ecking loop

FIX: Remove unrelated float16() casts from tests

4b1c035

FIX: Don't say dictionary<string_view> casts are supported yet

72decf7

Use 'Offset String' instead of 'Span'

f0043f2

Mention issue 43573

9ad5587

Check against overflow of fixed-size string offsets

ea022c7

Improve comment listing types that have std::string_view C-type

3c418d5

Calculate how much data will be needed to convert view to offset string

4d51841

Fix the Offset String -> String View issue

2e47828

Check length for Offset String -> String View

f4c8d1f

resolve comment

b17e170

apply suggestions

2416d19

pitrou force-pushed the str2str_casts branch from 02d7467 to 2416d19 Compare September 12, 2024 15:44

pitrou merged commit 85fc3eb into apache:main Sep 12, 2024

pitrou removed the awaiting change review Awaiting change review label Sep 12, 2024

felipecrv deleted the str2str_casts branch September 12, 2024 18:24

jorisvandenbossche mentioned this pull request Oct 28, 2024

[C++] Implement basic functionality for new Binary/Utf8View types #39634

Open

6 tasks

fwojciec mentioned this pull request Nov 8, 2024

unsupported cast to string_view from utf8 in v18 apache/arrow-go#184

Open

crystalxyz mentioned this pull request Nov 24, 2024

GH-43573: [C++] Copy bitmap when casting from string-view to offset string and binary types #44822

Merged

GH-42247: [C++] Support casting to and from utf8_view/binary_view #43302

GH-42247: [C++] Support casting to and from utf8_view/binary_view #43302

Uh oh!

Conversation

felipecrv commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

felipecrv Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

felipecrv Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mapleFU Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

bkietz Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

bkietz Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 12, 2024

Uh oh!

pitrou commented Sep 12, 2024

Uh oh!

mapleFU commented Sep 13, 2024

Uh oh!

conbench-apache-arrow bot commented Sep 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felipecrv commented Jul 17, 2024 •

edited

Loading

pitrou commented Sep 12, 2024 •

edited

Loading