GH-36099: [C++] Add Utf8View and BinaryView to the c ABI #38443

bkietz · 2023-10-24T20:30:17Z

Rationale for this change

Utf8View and BinaryView should be added to the c ABI spec and to the c++ library's importer/exporter.

Are these changes tested?

Yes, minimally

Are there any user-facing changes?

View arrays will be importable/exportable through the c ABI in c++

Closes: Add string view to the C ABI #36099

bkietz

Note: based on #37792

cpp/src/arrow/c/bridge_test.cc

felipecrv · 2023-10-24T22:11:06Z

LGTM. But of course we need to wait for the separate review and merge of the string view PR before merging this.

pitrou · 2023-10-25T08:06:25Z

You also need to remove the corresponding skips in datagen.py.

bkietz · 2023-10-25T14:59:18Z

The largest issue I had forgotten: C-ABI doesn't store buffer sizes explicitly. (For string data we derive the data buffer's length by reading the offsets buffer.) I guess we'll need an extra buffer to store those, since reconstructing the sizes by examining every view is untenable

bkietz · 2023-11-09T16:08:38Z

cpp/src/arrow/c/bridge_test.cc

@@ -3986,8 +4072,6 @@ TEST_F(TestDeviceArrayRoundtrip, Primitive) {
  TestWithJSON(mm, int32(), "[4, 5, null]");
 }

-// TODO C -> C++ -> C roundtripping tests?


I think this can be removed now since the C abi integration tests should cover this

bkietz · 2023-11-28T17:39:58Z

ping @pitrou

zeroshade

just a few nitpicks / comments

zeroshade · 2023-11-28T21:34:09Z

cpp/src/arrow/c/bridge.cc

-      dict_exporter_.reset(new SchemaExporter());
+      dict_exporter_ = std::make_unique<SchemaExporter>();


i've actually wondered.... is there any particular difference (performance or otherwise) doing .reset(new T) vs = std::make_unique<T>()?

In scenarios where you have an std::unique_ptr<ParentClass> being populated by many sub-classes of ParentClass:

std::make_unique<T, Deleter>(...) can lead to more binary bloat because class Deleter = std::default_delete<T> is type-specialized whereas reset() takes a super-class pointer and only needs the deleter that invokes the destructor via dynamic dispatching through the parent class vtable.

Being type-specialized often leads to the derived-classes destructors' being inlined into the std::default_deleter<T>. All this extra inlining and removal of one indirection could lead to less overhead of destructor dispatching.

In that case, wouldn't it be better to keep this as calling .reset rather than switch to make_unique?

In this case there is no difference since the assigned pointer is a unique_ptr<SchemaExporter> and not a pointer to a base/parent class.

cpp/src/arrow/c/bridge.cc

cpp/src/arrow/c/bridge_test.cc

pitrou

I see no array roundtrip test, am I missing something?
Can you rebase to get the latest changes (including list view integration)?

cpp/src/arrow/c/bridge_test.cc

bkietz · 2023-11-29T16:01:49Z

@pitrou rebased, added round trip tests

pitrou

LGTM, but I think there are lint errors to fix

felipecrv · 2023-11-29T19:09:32Z

Yay! 🚀

conbench-apache-arrow · 2023-12-01T00:23:53Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 94fc124.

There were no benchmark performance regressions. 🎉

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

urvishdesai · 2023-12-01T01:16:58Z

Hi @bkietz, is there a plan to add Utf8View support to the rust arrow2 library soon?

If someone is not already working on it, I would like to take up the PR to add support for Utf8View to the arrow2 library (jorgecarleitao/arrow2#1596)

bkietz · 2023-12-01T12:10:51Z

@urvishdesai thanks for offering! I don't think anyone else has that queued, please do (and I'd be happy to review)

…e#38443) ### Rationale for this change Utf8View and BinaryView should be added to the c ABI spec and to the c++ library's importer/exporter. ### Are these changes tested? Yes, minimally ### Are there any user-facing changes? View arrays will be importable/exportable through the c ABI in c++ * Closes: apache#36099 Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

bkietz requested review from assignUser, kou, raulcd, paleolimbot, thisisnic, wgtmac and westonpace as code owners October 24, 2023 20:30

github-actions bot added Component: R Component: Parquet Component: C++ Component: Python Component: Documentation awaiting committer review Awaiting committer review labels Oct 24, 2023

bkietz commented Oct 24, 2023

View reviewed changes

cpp/src/arrow/c/bridge_test.cc Show resolved Hide resolved

bkietz requested review from felipecrv and pitrou October 24, 2023 20:34

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 24, 2023

bkietz marked this pull request as draft October 25, 2023 14:56

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 25, 2023

bkietz force-pushed the 36099-string-view-c-abi branch from 0afb739 to 2d13898 Compare November 1, 2023 18:30

github-actions bot removed Component: R Component: Parquet Component: Python labels Nov 1, 2023

bkietz marked this pull request as ready for review November 2, 2023 16:49

bkietz commented Nov 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 9, 2023

felipecrv removed their request for review November 28, 2023 18:28

zeroshade reviewed Nov 28, 2023

View reviewed changes

pitrou reviewed Nov 29, 2023

View reviewed changes

cpp/src/arrow/c/bridge_test.cc Outdated Show resolved Hide resolved

pitrou requested changes Nov 29, 2023

View reviewed changes

cpp/src/arrow/c/bridge_test.cc Outdated Show resolved Hide resolved

bkietz added 3 commits November 29, 2023 10:59

apacheGH-36099: [C++] Add Utf8View and BinaryView to the c data bridge

75d5395

Test import/export of string view data with multiple data buffers

14db1ce

review comments

df068c2

bkietz force-pushed the 36099-string-view-c-abi branch from 0b73842 to df068c2 Compare November 29, 2023 16:01

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 29, 2023

pitrou approved these changes Nov 29, 2023

View reviewed changes

lint

bcd9d54

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 29, 2023

bkietz merged commit 94fc124 into apache:main Nov 29, 2023
35 of 37 checks passed

bkietz removed the awaiting changes Awaiting changes label Nov 29, 2023

bkietz deleted the 36099-string-view-c-abi branch November 29, 2023 17:22

jorisvandenbossche mentioned this pull request Jan 17, 2024

feat(rust): BinaryView/Utf8View IPC support pola-rs/polars#13464

Merged

eitsupi mentioned this pull request Feb 6, 2024

Export Utf8View type via Arrow C stream pola-rs/r-polars#781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36099: [C++] Add Utf8View and BinaryView to the c ABI #38443

GH-36099: [C++] Add Utf8View and BinaryView to the c ABI #38443

bkietz commented Oct 24, 2023 •

edited by github-actions bot

Loading

bkietz left a comment

felipecrv commented Oct 24, 2023

pitrou commented Oct 25, 2023

bkietz commented Oct 25, 2023

bkietz Nov 9, 2023

bkietz commented Nov 28, 2023

zeroshade left a comment

zeroshade Nov 28, 2023

felipecrv Nov 28, 2023

zeroshade Nov 29, 2023

bkietz Nov 29, 2023

pitrou left a comment

bkietz commented Nov 29, 2023

pitrou left a comment

felipecrv commented Nov 29, 2023

conbench-apache-arrow bot commented Dec 1, 2023

urvishdesai commented Dec 1, 2023 •

edited

Loading

bkietz commented Dec 1, 2023

		dict_exporter_.reset(new SchemaExporter());
		dict_exporter_ = std::make_unique<SchemaExporter>();

GH-36099: [C++] Add Utf8View and BinaryView to the c ABI #38443

GH-36099: [C++] Add Utf8View and BinaryView to the c ABI #38443

Conversation

bkietz commented Oct 24, 2023 • edited by github-actions bot Loading

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

bkietz left a comment

Choose a reason for hiding this comment

felipecrv commented Oct 24, 2023

pitrou commented Oct 25, 2023

bkietz commented Oct 25, 2023

bkietz Nov 9, 2023

Choose a reason for hiding this comment

bkietz commented Nov 28, 2023

zeroshade left a comment

Choose a reason for hiding this comment

zeroshade Nov 28, 2023

Choose a reason for hiding this comment

felipecrv Nov 28, 2023

Choose a reason for hiding this comment

zeroshade Nov 29, 2023

Choose a reason for hiding this comment

bkietz Nov 29, 2023

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

bkietz commented Nov 29, 2023

pitrou left a comment

Choose a reason for hiding this comment

felipecrv commented Nov 29, 2023

conbench-apache-arrow bot commented Dec 1, 2023

urvishdesai commented Dec 1, 2023 • edited Loading

bkietz commented Dec 1, 2023

bkietz commented Oct 24, 2023 •

edited by github-actions bot

Loading

urvishdesai commented Dec 1, 2023 •

edited

Loading