pitrou commented on a change in pull request #11197:
URL: https://github.com/apache/arrow/pull/11197#discussion_r716602151
##########
File path: cpp/examples/arrow/row_wise_conversion_example.cc
##########
@@ -47,79 +53,85 @@ struct data_row {
// construction of the final `arrow::Array` instances.
//
// For each type, Arrow has a specially typed builder class. For the primitive
-// values `id` and `cost` we can use the respective `arrow::Int64Builder` and
-// `arrow::DoubleBuilder`. For the `cost_components` vector, we need to have
two
-// builders, a top-level `arrow::ListBuilder` that builds the array of offsets
and
-// a nested `arrow::DoubleBuilder` that constructs the underlying values array
that
+// values `id` and `components` we can use the `arrow::Int64Builder`. For the
+// `component_cost` vector, we need to have two builders, a top-level
+// `arrow::ListBuilder` that builds the array of offsets and a nested
+// `arrow::DoubleBuilder` that constructs the underlying values array that
// is referenced by the offsets in the former array.
-arrow::Status VectorToColumnarTable(const std::vector<struct data_row>& rows,
- std::shared_ptr<arrow::Table>* table) {
+arrow::Result<std::shared_ptr<arrow::Table>> VectorToColumnarTable(
+ const std::vector<struct data_row>& rows) {
// The builders are more efficient using
// arrow::jemalloc::MemoryPool::default_pool() as this can increase the size
of
// the underlying memory regions in-place. At the moment, arrow::jemalloc is
only
// supported on Unix systems, not Windows.
arrow::MemoryPool* pool = arrow::default_memory_pool();
Int64Builder id_builder(pool);
- DoubleBuilder cost_builder(pool);
- ListBuilder components_builder(pool, std::make_shared<DoubleBuilder>(pool));
- // The following builder is owned by components_builder.
- DoubleBuilder& cost_components_builder =
- *(static_cast<DoubleBuilder*>(components_builder.value_builder()));
+ Int64Builder components_builder(pool);
+ ListBuilder component_cost_builder(pool,
std::make_shared<DoubleBuilder>(pool));
+ // The following builder is owned by component_cost_builder.
+ DoubleBuilder* component_item_cost_builder =
+ (static_cast<DoubleBuilder*>(component_cost_builder.value_builder()));
// Now we can loop over our existing data and insert it into the builders.
The
// `Append` calls here may fail (e.g. we cannot allocate enough additional
memory).
// Thus we need to check their return values. For more information on these
values,
// check the documentation about `arrow::Status`.
for (const data_row& row : rows) {
ARROW_RETURN_NOT_OK(id_builder.Append(row.id));
- ARROW_RETURN_NOT_OK(cost_builder.Append(row.cost));
+ ARROW_RETURN_NOT_OK(components_builder.Append(row.components));
// Indicate the start of a new list row. This will memorise the current
// offset in the values builder.
- ARROW_RETURN_NOT_OK(components_builder.Append());
- // Store the actual values. The final nullptr argument tells the underlying
- // builder that all added values are valid, i.e. non-null.
-
ARROW_RETURN_NOT_OK(cost_components_builder.AppendValues(row.cost_components.data(),
-
row.cost_components.size()));
+ ARROW_RETURN_NOT_OK(component_cost_builder.Append());
+ // Store the actual values. The same memory layout is
+ // used for the component cost data, in this case a vector of
+ // type double, as for the memory that Arrow uses to hold this
+ // data and will be created. The final nullptr argument tells
+ // the underlying builder that all added values are valid, i.e. non-null.
Review comment:
I know this is kept from the original version, but "the final nullptr
argument" doesn't seem to point to anything. Should we remove that mention?
##########
File path: cpp/examples/arrow/row_wise_conversion_example.cc
##########
@@ -176,15 +188,49 @@ arrow::Status ColumnarTableToVector(const
std::shared_ptr<arrow::Table>& table,
int main(int argc, char** argv) {
std::vector<data_row> rows = {
- {1, 1.0, {1.0}}, {2, 2.0, {1.0, 2.0}}, {3, 3.0, {1.0, 2.0, 3.0}}};
-
+ {1, 1, {10.0}}, {2, 3, {11.0, 12.0, 13.0}}, {3, 2, {15.0, 25.0}}};
std::shared_ptr<arrow::Table> table;
- EXIT_ON_FAILURE(VectorToColumnarTable(rows, &table));
-
std::vector<data_row> expected_rows;
- EXIT_ON_FAILURE(ColumnarTableToVector(table, &expected_rows));
- assert(rows.size() == expected_rows.size());
+ arrow::Result<std::shared_ptr<arrow::Table>> table_result =
+ VectorToColumnarTable(rows);
+ if(table_result.ok()){
+ table = std::move(table_result).ValueOrDie();
Review comment:
Can you try to follow the [coding
conventions](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)?
Specifically:
* use 2-space indentation
* use whitespace adequately around keyword and operators, for example `if
(table_result.ok()) {` and not `if(table_result.ok()){`
##########
File path: cpp/examples/arrow/row_wise_conversion_example.cc
##########
@@ -176,15 +188,49 @@ arrow::Status ColumnarTableToVector(const
std::shared_ptr<arrow::Table>& table,
int main(int argc, char** argv) {
std::vector<data_row> rows = {
- {1, 1.0, {1.0}}, {2, 2.0, {1.0, 2.0}}, {3, 3.0, {1.0, 2.0, 3.0}}};
-
+ {1, 1, {10.0}}, {2, 3, {11.0, 12.0, 13.0}}, {3, 2, {15.0, 25.0}}};
std::shared_ptr<arrow::Table> table;
- EXIT_ON_FAILURE(VectorToColumnarTable(rows, &table));
-
std::vector<data_row> expected_rows;
- EXIT_ON_FAILURE(ColumnarTableToVector(table, &expected_rows));
- assert(rows.size() == expected_rows.size());
+ arrow::Result<std::shared_ptr<arrow::Table>> table_result =
+ VectorToColumnarTable(rows);
+ if(table_result.ok()){
+ table = std::move(table_result).ValueOrDie();
+ arrow::Result<std::vector<data_row>> expected_rows_result =
+ ColumnarTableToVector(table);
+ if(expected_rows_result.ok()) {
+ expected_rows =
+ std::move(expected_rows_result).ValueOrDie();
Review comment:
Since we're using `ValueOrDie`, it's obvious that an error will simply
abort the process with a message. Consequently, we don't need to test for the
result being ok. This should simplify the example quite a bit, for example:
```c++
arrow::Result<std::shared_ptr<arrow::Table>> table_result =
VectorToColumnarTable(rows);
table = std::move(table_result).ValueOrDie();
// etc.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
