ARROW-16006: [C++][Docs] Provide row conversion example for dynamic schemas #12775

wjones127 · 2022-04-01T22:43:00Z

This PR is meant to improve the ease of converting between Arrow tabular structure and row-based data. It adds an example of implementing and using the row converters to convert between vectors of rapidjson::Document and Arrow tables, highlighting some common patterns and important utilities provided by Arrow C++.

github-actions · 2022-04-01T22:43:20Z

https://issues.apache.org/jira/browse/ARROW-16006

wjones127 · 2022-04-29T21:38:02Z

Not sure if this is related to the appveyor segfault, but some batch sizes produce a segfault:

# segfault for me
rapidjson-row-converter 1000 500

# runs fine
rapidjson-row-converter 1000 800

wjones127 · 2022-06-03T16:06:08Z

Just rebased. CI failure is unrelated.

@emkornfield @pitrou Would one of you be willing to review soon?

pitrou · 2022-06-09T12:11:56Z

@wjones127 I don't know if that would be painful or not, but if not, would you like to submit the RecordBatchBuilder API changes in a separate PR that we can merge quickly?

pitrou · 2022-06-09T12:14:16Z

cpp/src/arrow/row_conversion.h

+
+/// \brief Base class for Arrow to row converter.
+///
+/// To use, specialize the conversion for a particular row type.


Hmm... so that means that for a given RowType I cannot implement two different converters (for different actual formats) because symbols would clash. That sounds a bit limiting.

Ah, I hadn't seen that you had to subclass in addition to specializing the class. My bad.

pitrou · 2022-06-09T12:16:32Z

I don't know what the exact requirements for this are, but if performance is not important then do note there are already conversion utilities in stl.h that go back and forth between arrow::Table and std::vector<std::tuple<...>>.

pitrou · 2022-06-09T12:29:45Z

Generally I have mixed feelings about exposing and maintaing this API. On the one hand it can ease writing conversions a bit, on the other hand it doesn't really abstract the task a lot anyway (mostly it seems to translate between iterator <-> record batch sequence <-> table).

Perhaps what we need is simpler ways of writing the translations mentioned above?

cc @bkietz for opinions

wjones127 · 2022-06-09T16:18:20Z

I don't know what the exact requirements for this are, but if performance is not important then do note there are already conversion utilities in stl.h that go back and forth between arrow::Table and std::vector<std::tuple<...>>.

One of the intended use case is designing performant connectors for NoSQL databases, who typically provide results as a vector or iterator of row / document objects. For example, DynamoDB's C++ SDK returns Aws::Map<Aws::String, Aws::DynamoDB::Model::AttributeValue>. Cassandra has a similar class. More generally, there are a lot of existing systems that work off of rows that an adopter of Arrow may need to connect to.

Looking that the utilities in stl.h, one thing I haven't explored is whether arrow::stl::ConversionTraits<T> and arrow::CTypeTraits<T>.

Generally I have mixed feelings about exposing and maintaining this API. On the one hand it can ease writing conversions a bit, on the other hand it doesn't really abstract the task a lot anyway (mostly it seems to translate between iterator <-> record batch sequence <-> table).

I agree that the new API doesn't do that much on the task; the translation it does between iterator, RB, and tables is mostly for convenience. The main thing it does is establish a pattern of converting one batch at a time, and potentially operating on a smaller batch size than a table is currently grouped in for cases where the inner conversion loops performs better with smaller batches.

It's hard to make an API that general enough that handles most of the conversion; that's why most of this PR is an example demonstrating how to use Arrow's VisitXInline methods, type traits, and conversions to C types to implement row conversions. Those features make writing the row-conversions a lot easier, but they aren't well-documented for end users. If we think the new API doesn't add much, I could just fold its logic into the example and this could just be a docs PR 🤷 .

wjones127 · 2022-06-09T16:18:58Z

@wjones127 I don't know if that would be painful or not, but if not, would you like to submit the RecordBatchBuilder API changes in a separate PR that we can merge quickly?

Yes, I should do that. I was being lazy not making a separate PR.

pitrou · 2022-06-09T16:21:12Z

cc @lidavidm for opinions on the NoSQL connector use case.

lidavidm · 2022-06-09T18:10:36Z

docs/source/cpp/examples/row_columnar_conversion.rst

+
+For schemas not known at compile time, implement an :class:`arrow::ToRowConverter`
+or an :class:`arrow::FromRowConverter`.
+The following example shows how to implement conversion between `rapidjson::Document`


Suggested change

The following example shows how to implement conversion between `rapidjson::Document`

The following example shows how to implement conversion between ``rapidjson::Document``

lidavidm · 2022-06-09T18:17:15Z

cpp/examples/arrow/rapidjson_row_converter.cc

+
+  arrow::Status Visit(const arrow::ListArray& array) {
+    // First create rows from values
+    std::shared_ptr<arrow::Array> values = array.values();


While minor, I wonder why BaseListArray::values doesn't return const Array& or at least const std::shared_ptr<Array>&…not that it should be a big deal here.

cpp/examples/arrow/rapidjson_row_converter.cc

lidavidm · 2022-06-09T18:18:57Z

cpp/src/arrow/row_conversion.h

+  /// \param batch Record batch to convert
+  /// \return A vector of rows containing all data in batch
+  virtual Result<ContainerType<RowType>> ConvertToVector(
+      std::shared_ptr<RecordBatch> batch) = 0;


Why not take const RecordBatch& or const std::shared_ptr<RecordBatch>&?

lidavidm · 2022-06-09T18:19:48Z

cpp/examples/arrow/rapidjson_row_converter.cc

+    RowBatchBuilder builder{batch->num_rows()};
+
+    for (int i = 0; i < batch->num_columns(); ++i) {
+      builder.SetField(std::move(batch->schema()->field(i)));


nit: I don't think this std::move does anything here

lidavidm · 2022-06-09T18:21:38Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  for (const rapidjson::Document& doc : records) {
+    rapidjson::StringBuffer sb;
+    rapidjson::Writer<rapidjson::StringBuffer> writer(sb);
+    // At batch_size >= 495, we segfault here:


Ah that's old. IIRC there was a bug in my handling of the rapidjson allocators.

lidavidm · 2022-06-09T18:22:21Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  // Convert records into a table
+  arrow::Result<std::shared_ptr<arrow::Table>> table_result =
+      to_arrow_converter.ConvertToTable(records, schema);
+  std::shared_ptr<arrow::Table> table = std::move(table_result).ValueOrDie();


in this case why not just table = *to_arrow_converter.ConvertToTable();?

or, we could factor out a Status Main() that gets called from int main

I like the idea of a Status Main(), since that enables us to show use of result/status macros in the example code blocks 👍

lidavidm · 2022-06-09T18:30:56Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  // Default implementation
+  arrow::Status Visit(const arrow::DataType& type) {
+    return arrow::Status::NotImplemented(
+        "Can not convert to json document for array of type ", type.ToString());


Cannot convert to Arrow array?

Yup, thanks!

lidavidm · 2022-06-09T18:32:03Z

cpp/examples/arrow/CMakeLists.txt

@@ -17,6 +17,8 @@

 add_arrow_example(row_wise_conversion_example)

+add_arrow_example(rapidjson_row_converter)


wrap in if(ARROW_JSON)?

I'm also surprised you don't have to link to rapidjson explicitly…

I think rapidjson is header-only. I didn't see it linked anywhere in CMakeLists for ARROW_JSON.

Oh, good to know.

That said, if(ARROW_JSON) would still be good so that this doesn't fail building if RapidJSON wasn't required for building Arrow itself.

lidavidm · 2022-06-09T18:33:39Z

cpp/src/arrow/row_conversion.h

+    ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Table> table,
+                          Table::FromRecordBatches({batch}));
+    return ConvertToVector(table, batch_size);


IMO, it's pointless to wrap in a table only to unwrap it right away

emkornfield · 2022-06-11T17:16:11Z

c_glib/arrow-glib/table-builder.cpp

+  auto builder_result = arrow::RecordBatchBuilder::Make(arrow_schema, memory_pool);
+
+  if (garrow::check(error, builder_result, "[record-batch-builder][new]")) {
+    std::unique_ptr<arrow::RecordBatchBuilder> arrow_builder = std::move(builder_result).ValueOrDie();


nit: formatting, as is this change related?

It's not related. I've moved this change to #13356 and will rebase after that's merged.

emkornfield · 2022-06-11T17:17:22Z

cpp/examples/arrow/rapidjson_row_converter.cc

+//  * conversion for Arrow types that have corresponding C types (bool, integer,
+//    float).
+
+rapidjson::Value kNullJsonSingleton = rapidjson::Value();


nit: make this const static?

emkornfield · 2022-06-11T17:20:01Z

cpp/examples/arrow/rapidjson_row_converter.cc

+      const ArrayType& array) {
+    for (int64_t i = 0; i < array.length(); ++i) {
+      if (!array.IsNull(i)) {
+        rapidjson::Value str_key(field_->name().c_str(), rows_[i].GetAllocator());


nit: c_str,looks wrong here (what about unicode field names)

rapidjson doesn't accept std::string, it only accepts const char*. The character encoding is part of the value type definition: rapidjson::Value = rapidjson::GenericValue<UTF8<> >. So I think unicode should be correctly handled here.

It looks like there is a StringRef that can take a length.

Actually, it turns out if I #define RAPIDJSON_HAS_STDSTRING 1, rapidjson will just accept std::string 🤦 . Removed all the .c_str() 😄

emkornfield · 2022-06-13T03:52:48Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  arrow::Status Visit(const arrow::StringArray& array) {
+    for (int64_t i = 0; i < array.length(); ++i) {
+      if (!array.IsNull(i)) {
+        rapidjson::Value str_key(field_->name().c_str(), rows_[i].GetAllocator());


same comment about c_str.

emkornfield · 2022-06-13T03:53:38Z

cpp/examples/arrow/rapidjson_row_converter.cc

+    for (int64_t i = 0; i < array.length(); ++i) {
+      if (!array.IsNull(i)) {
+        rapidjson::Value str_key(field_->name().c_str(), rows_[i].GetAllocator());
+        // StringArray.Value returns a string view


it seems it would be more succinct to spell out the type below then to have this comment.

emkornfield · 2022-06-13T03:55:07Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  arrow::Status Visit(const arrow::StructArray& array) {
+    const arrow::StructType* type = array.struct_type();
+
+    RowBatchBuilder child_builder(rows_.size());


should this be based on array.size()?

or is this saying only extract n_rows from the struct array?

The expectation is that the class is constructed with the correct number of rows, so rows_.size() == array.length(). But I think it's worth adding an assert here to make that explicit.

lidavidm · 2022-06-13T11:03:38Z

Oops, I didn't actually reply to the use case part.

One of the intended use case is designing performant connectors for NoSQL databases, who typically provide results as a vector or iterator of row / document objects. For example, DynamoDB's C++ SDK returns Aws::Map<Aws::String, Aws::DynamoDB::Model::AttributeValue>. Cassandra has a similar class. More generally, there are a lot of existing systems that work off of rows that an adopter of Arrow may need to connect to.

It would be useful to make this use case easier, but I'm still not sure if it needs to be a new API, vs a code sample or suggestions on how to approach it. row_conversion.h here is very minimal and could frankly just be copy-pasted into an application easily enough without having to make it a public Arrow API. That said I don't necessarily object to having it if it's going to be a common enough pattern and/or if that makes it easy to point to.

emkornfield · 2022-06-13T16:01:33Z

cpp/examples/arrow/rapidjson_row_converter.cc

+    for (int64_t i = 0; i < array.length(); ++i) {
+      rapidjson::Document::AllocatorType& allocator = rows_[i].GetAllocator();
+      auto array_len = array.value_length(i);
+


shouldn't there be a null check someplace in here?

Ah, yes there should be. Thanks!

Refactor to use record batch builder Fix most compiler errors Get a compiling version Allow overriding container type A bit more progress Draft list handling Get parsing working Get example running all the way through Draft narrative documentation Enhance docs Remove unused line Fix lint issues Fix ORC references Move at end Fix integer types Use sizetype from rj Use static cast Format Setup for benchmarks Start debugging Always retrieve the correct allocator Must copy value to new allocator Better handling of unique ptr Format Use better template and pure virtual functions

wjones127 · 2022-06-22T18:52:15Z

I have refactored this PR to only create an example, rebased so that the RecordBatchBuilder changes are separated, and rewrote most of the prose in the example.

lidavidm · 2022-06-22T23:50:08Z

cpp/examples/arrow/rapidjson_row_converter.cc

+  template <typename ArrayType, typename DataClass = typename ArrayType::TypeClass>
+  arrow::enable_if_primitive_ctype<DataClass, arrow::Status> Visit(
+      const ArrayType& array) {
+    assert((int64_t)rows_.size() == array.length());


nit: prefer C++-style cast? (here and below)

lidavidm · 2022-06-22T23:53:05Z

Do we also want to put the example in the Cookbook somehow? I feel like it's a bit too long/involved to go there directly, but maybe we can cross-reference it.

wjones127 · 2022-06-23T00:56:18Z

Do we also want to put the example in the Cookbook somehow? I feel like it's a bit too long/involved to go there directly, but maybe we can cross-reference it.

Yeah, I can create a link to it somewhere under the "Creating Arrow Objects" section I think.

pitrou

This LGTM though I'm unsure how many people would actually follow such a long and non-trivial example.

wjones127 · 2022-06-28T22:09:59Z

This LGTM though I'm unsure how many people would actually follow such a long and non-trivial example.

It's a common use case and I tried to add narrative docs that broke up the example to highlight useful utilities. But it is unfortunate we weren't able to find any general abstractions to make this operation short and trivial.

I think this is ready to be merged, unless @emkornfield has any objections?

emkornfield · 2022-07-03T19:18:14Z

sorry never got through a full review so if other reviewers are happy with it I'm fine checking it in. Is there a summary for why we don't think there is a path forward for a generalized method?

wjones127 · 2022-07-05T21:13:57Z

Is there a summary for why we don't think there is a path forward for a generalized method?

Here it came down to the abstraction we did come up with didn't do very much; most of the value was in the example. In the way it was designed, we asked the user to implement the conversion of one batch of rows to one RecordBatch, and the API took care of the rest. But in our example, most of the logic was within that single batch conversion example implementation. It's possible with another pass we could come up a worthwhile API; perhaps if the user-implemented hooks were narrower in scope.

github-actions bot added the Component: C++ label Apr 1, 2022

emkornfield self-requested a review April 21, 2022 17:36

wjones127 force-pushed the ARROW-16006-row-conversion branch from 252e3eb to ca87369 Compare April 22, 2022 19:07

wjones127 changed the title ~~ARROW-16006: [C++] Row conversion helpers and example [WIP]~~ ARROW-16006: [C++] Row conversion helpers and example Apr 28, 2022

github-actions bot added the Component: GLib label Apr 29, 2022

wjones127 force-pushed the ARROW-16006-row-conversion branch from e625503 to b0b140b Compare April 29, 2022 21:38

wjones127 marked this pull request as ready for review May 5, 2022 23:36

wjones127 force-pushed the ARROW-16006-row-conversion branch from 15cea97 to f6f1eaa Compare May 17, 2022 18:58

wjones127 force-pushed the ARROW-16006-row-conversion branch 2 times, most recently from 487323c to b627507 Compare June 3, 2022 14:41

pitrou reviewed Jun 9, 2022

View reviewed changes

lidavidm reviewed Jun 9, 2022

View reviewed changes

emkornfield reviewed Jun 11, 2022

View reviewed changes

emkornfield reviewed Jun 13, 2022

View reviewed changes

github-actions bot added the Component: Documentation label Jun 14, 2022

wjones127 added 6 commits June 22, 2022 11:48

chore: respond to basic PR feedback

189100e

refactor: use RAPIDJSON_HAS_STDSTRING

c4b7a7d

fix: Add trailing newline

f2090ef

feat: Move API changes into example

516566d

feat: refactor narrative docs

c32ef47

wjones127 force-pushed the ARROW-16006-row-conversion branch from 50f9efd to c32ef47 Compare June 22, 2022 18:50

wjones127 requested review from lidavidm, emkornfield and pitrou June 22, 2022 19:26

lidavidm approved these changes Jun 22, 2022

View reviewed changes

fix: use static_cast instead of C-style cast

d84ef7f

wjones127 changed the title ~~ARROW-16006: [C++] Row conversion helpers and example~~ ARROW-16006: [C++][Docs] Provide row conversion example for dynamic schemas Jun 23, 2022

pitrou reviewed Jun 28, 2022

View reviewed changes

lidavidm merged commit 73588c3 into apache:master Jul 6, 2022

wjones127 deleted the ARROW-16006-row-conversion branch July 6, 2022 14:28

asfimport mentioned this pull request Nov 23, 2022

[C++][Docs] Provide row conversion example for dynamic schemas #31429

Closed

	The following example shows how to implement conversion between `rapidjson::Document`
	The following example shows how to implement conversion between ``rapidjson::Document``

		@@ -17,6 +17,8 @@

		add_arrow_example(row_wise_conversion_example)

		add_arrow_example(rapidjson_row_converter)

ARROW-16006: [C++][Docs] Provide row conversion example for dynamic schemas #12775

ARROW-16006: [C++][Docs] Provide row conversion example for dynamic schemas #12775

Conversation

wjones127 commented Apr 1, 2022 • edited

github-actions bot commented Apr 1, 2022

wjones127 commented Apr 29, 2022

wjones127 commented Jun 3, 2022

pitrou commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jun 9, 2022

pitrou commented Jun 9, 2022 • edited

wjones127 commented Jun 9, 2022

wjones127 commented Jun 9, 2022

pitrou commented Jun 9, 2022

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

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

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

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

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

lidavidm commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 commented Jun 22, 2022

Choose a reason for hiding this comment

lidavidm commented Jun 22, 2022

wjones127 commented Jun 23, 2022

pitrou left a comment

Choose a reason for hiding this comment

wjones127 commented Jun 28, 2022

emkornfield commented Jul 3, 2022

wjones127 commented Jul 5, 2022

wjones127 commented Apr 1, 2022 •

edited

pitrou commented Jun 9, 2022 •

edited