You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ko...@apache.org on 2023/06/27 01:50:47 UTC
[arrow] branch main updated: GH-36249: [MATLAB] Create a `MATLAB_ASSIGN_OR_ERROR` macro to mirror the C++ `ARROW_ASSIGN_OR_RAISE` macro (#36273)
This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new d5d98ae676 GH-36249: [MATLAB] Create a `MATLAB_ASSIGN_OR_ERROR` macro to mirror the C++ `ARROW_ASSIGN_OR_RAISE` macro (#36273)
d5d98ae676 is described below
commit d5d98ae676194b74a40434c3cac96eb6c3dc31fd
Author: Kevin Gurney <kg...@mathworks.com>
AuthorDate: Mon Jun 26 21:50:40 2023 -0400
GH-36249: [MATLAB] Create a `MATLAB_ASSIGN_OR_ERROR` macro to mirror the C++ `ARROW_ASSIGN_OR_RAISE` macro (#36273)
### Rationale for this change
1. To make working with `arrow::Result<T>` values easier in the C++ code for the MATLAB interface, it would be useful to have a `MATLAB_ASSIGN_OR_ERROR` macro that mirrors the design of the [`ARROW_ASSIGN_OR_RAISE`](https://github.com/apache/arrow/blob/320ecbd119f26cb2f8d604ed84aae2559dbc0e26/cpp/src/arrow/result.h#L475) macro.
2. @ kou helpfully suggested this here: https://github.com/apache/arrow/pull/36190#discussion_r1237932111.
3. There is already a [`MATLAB_ERROR_IF_NOT_OK`](https://github.com/apache/arrow/blob/e3eb5898e75a0b901724f771a7e2de069993a33c/matlab/src/cpp/arrow/matlab/error/error.h#L26) macro, so this would roughly follow the approach of that macro.
### What changes are included in this PR?
1. Added new macros `MATLAB_ASSIGN_OR_ERROR(lhs, rexpr, id)`, `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(lhs, rexpr, context, id)`, and `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(expr, context, id)`.
2. Added additional comments describing the error macros.
3. Updated call sites (i.e. `numeric_array.h`, `boolean_array.cc`, and `record_batch.cc`) where `arrow::Result<T>` is being returned to use new `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros where possible.
**Example**
*`MATLAB_ASSIGN_OR_ERROR`*
```matlab
// Erroring from a static Proxy "make" member function
libmexclass::proxy::MakeResult CustomProxy::make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
...
MATLAB_ASSIGN_OR_ERROR(auto array, array_builder.Finish(), "arrow:matlab:array:builder:FailedToBuildArray");
...
}
```
*`MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT`*
```matlab
// Erroring from a non-static Proxy member function
void CustomProxy::example(libmexclass::proxy::method::Context& context) {
...
// context is passed in as an input argument to the non-static Proxy member function
MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(auto array, array_builder.Finish(), context, "arrow:matlab:array:builder:FailedToBuildArray");
...
}
```
### Are these changes tested?
Yes.
1. The client code that was changed to use `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` compiles and works as expected.
2. **Note**: The `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT` and `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros are intended to be used with *non-static* `Proxy` member functions. For reference, non-static `Proxy` member functions return an error ID to MATLAB by assigning to the `context.error` property of the input `libmexclass::proxy::method::Context` object (see [this example](https://github.com/mathworks/libmexclass/blob/77f3d72c22a9ddab7b54ba325d757c3e82e57987/example/proxy/Car.cpp#L46)) [...]
### Are there any user-facing changes?
Yes.
1. There are now new `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros that can be used to simplify the process of extracting a value of type `T` from an expression that returns an `arrow::Result<T>` or returning an appropriate error to MATLAB if the status of the `Result` is not "OK".
### Future Directions
1. Consider modifying `mathworks/libmexclass` so that non-static `Proxy` member functions return a `Result`-like object, instead of requiring clients to assign to `context.error`. This might help avoid the need for two different "kinds" of macros - i.e. `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` and `MATLAB_ERROR_IF_NOT_OK` / `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT`.
### Notes
1. Thank you @ sgilmore10 for your help with this pull request!
* Closes: #36249
Lead-authored-by: Kevin Gurney <kg...@mathworks.com>
Co-authored-by: Kevin Gurney <ke...@gmail.com>
Co-authored-by: Sarah Gilmore <sg...@mathworks.com>
Co-authored-by: Sutou Kouhei <ko...@cozmixng.org>
Co-authored-by: Sarah Gilomore <sg...@mathworks.com>
Signed-off-by: Sutou Kouhei <ko...@clear-code.com>
---
.../cpp/arrow/matlab/array/proxy/boolean_array.cc | 8 +-
.../cpp/arrow/matlab/array/proxy/numeric_array.h | 10 +-
matlab/src/cpp/arrow/matlab/error/error.h | 134 ++++++++++++++++++++-
.../cpp/arrow/matlab/tabular/proxy/record_batch.cc | 31 ++---
4 files changed, 143 insertions(+), 40 deletions(-)
diff --git a/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc b/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
index 4e068b8ef9..9a3b7ed4e2 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
@@ -31,17 +31,13 @@ namespace arrow::matlab::array::proxy {
const ::matlab::data::TypedArray<bool> validity_bitmap_mda = opts[0]["Valid"];
// Pack the logical data values.
- auto maybe_packed_logical_buffer = arrow::matlab::bit::pack(logical_mda);
- MATLAB_ERROR_IF_NOT_OK(maybe_packed_logical_buffer.status(), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
+ MATLAB_ASSIGN_OR_ERROR(auto data_buffer, bit::pack(logical_mda), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
// Pack the validity bitmap values.
- auto maybe_validity_bitmap_buffer = bit::packValid(validity_bitmap_mda);
- MATLAB_ERROR_IF_NOT_OK(maybe_validity_bitmap_buffer.status(), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
+ MATLAB_ASSIGN_OR_ERROR(const auto validity_bitmap_buffer, bit::packValid(validity_bitmap_mda), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
const auto data_type = arrow::boolean();
const auto array_length = logical_mda.getNumberOfElements();
- const auto validity_bitmap_buffer = *maybe_validity_bitmap_buffer;
- const auto data_buffer = *maybe_packed_logical_buffer;
auto array_data = arrow::ArrayData::Make(data_type, array_length, {validity_bitmap_buffer, data_buffer});
return std::make_shared<arrow::matlab::array::proxy::BooleanArray>(arrow::MakeArray(array_data));
diff --git a/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h b/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
index ef5422b14f..62c6d9dc26 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
@@ -77,10 +77,9 @@ class NumericArray : public arrow::matlab::array::proxy::Array {
auto status = builder.AppendValues(dt, numeric_mda.getNumberOfElements(), unpacked_validity_bitmap);
MATLAB_ERROR_IF_NOT_OK(status, error::APPEND_VALUES_ERROR_ID);
- auto maybe_array = builder.Finish();
- MATLAB_ERROR_IF_NOT_OK(maybe_array.status(), error::BUILD_ARRAY_ERROR_ID);
+ MATLAB_ASSIGN_OR_ERROR(auto array, builder.Finish(), error::BUILD_ARRAY_ERROR_ID);
- return std::make_shared<arrow::matlab::array::proxy::NumericArray<CType>>(*maybe_array);
+ return std::make_shared<arrow::matlab::array::proxy::NumericArray<CType>>(array);
} else {
const auto data_type = arrow::CTypeTraits<CType>::type_singleton();
@@ -90,10 +89,7 @@ class NumericArray : public arrow::matlab::array::proxy::Array {
auto data_buffer = std::make_shared<arrow::Buffer>(reinterpret_cast<const uint8_t*>(dt),
sizeof(CType) * numeric_mda.getNumberOfElements());
// Pack the validity bitmap values.
-
- auto maybe_buffer = bit::packValid(valid_mda);
- MATLAB_ERROR_IF_NOT_OK(maybe_buffer.status(), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
- auto packed_validity_bitmap = *maybe_buffer;
+ MATLAB_ASSIGN_OR_ERROR(auto packed_validity_bitmap, bit::packValid(valid_mda), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
auto array_data = arrow::ArrayData::Make(data_type, length, {packed_validity_bitmap, data_buffer});
return std::make_shared<arrow::matlab::array::proxy::NumericArray<CType>>(arrow::MakeArray(array_data));
}
diff --git a/matlab/src/cpp/arrow/matlab/error/error.h b/matlab/src/cpp/arrow/matlab/error/error.h
index bcb9d936a3..4102054b88 100644
--- a/matlab/src/cpp/arrow/matlab/error/error.h
+++ b/matlab/src/cpp/arrow/matlab/error/error.h
@@ -17,12 +17,30 @@
#pragma once
-
#include "arrow/status.h"
#include "libmexclass/error/Error.h"
-#include <string_view>
-
+//
+// MATLAB_ERROR_IF_NOT_OK(expr, id)
+//
+// --- Description ---
+//
+// A macro used to return an error to MATLAB if the arrow::Status returned
+// by the specified expression is not "OK" (i.e. error).
+//
+// **NOTE**: This macro should be used inside of the static make() member function for a
+// Proxy class. Use MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT inside of a non-static
+// Proxy member function.
+//
+// --- Arguments ---
+//
+// expr - expression that returns an arrow::Status (e.g. builder.Append(...))
+// id - MATLAB error ID string (const char* - "arrow:matlab:proxy:make:FailedConstruction")
+//
+// --- Example ---
+//
+// MATLAB_ERROR_IF_NOT_OK(builder.Append(...), error::BUILDER_FAILED_TO_APPEND);
+//
#define MATLAB_ERROR_IF_NOT_OK(expr, id) \
do { \
arrow::Status _status = (expr); \
@@ -31,6 +49,116 @@
} \
} while (0)
+//
+// MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(expr, context, id)
+//
+// --- Description ---
+//
+// A macro used to return an error to MATLAB if the arrow::Status returned
+// by the specified expression is not "OK" (i.e. error).
+//
+// **NOTE**: This macro should be used inside of a non-static member function of a
+// Proxy class which has a libmexclass::proxy::method::Context as an input argument.
+// Use MATLAB_ERROR_IF_NOT_OK inside of a Proxy static make() member function.
+//
+// --- Arguments ---
+//
+// expr - expression that returns an arrow::Status (e.g. builder.Append(...))
+// context - libmexclass::proxy::method::Context context input to a Proxy method
+// id - MATLAB error ID string (const char* - "arrow:matlab:proxy:make:FailedConstruction")
+//
+// --- Example ---
+//
+// MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(builder.Append(...), context, error::BUILDER_FAILED_TO_APPEND);
+//
+#define MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(expr, context, id) \
+ do { \
+ arrow::Status _status = (expr); \
+ if (!_status.ok()) { \
+ context.error = libmexclass::error::Error{id, _status.message()}; \
+ return; \
+ } \
+ } while (0)
+
+#define MATLAB_ASSIGN_OR_ERROR_NAME(x, y) \
+ ARROW_CONCAT(x, y)
+
+#define MATLAB_ASSIGN_OR_ERROR_IMPL(result_name, lhs, rexpr, id) \
+ auto&& result_name = (rexpr); \
+ MATLAB_ERROR_IF_NOT_OK(result_name.status(), id); \
+ lhs = std::move(result_name).ValueUnsafe();
+
+//
+// MATLAB_ASSIGN_OR_ERROR(lhs, rexpr, id)
+//
+// --- Description ---
+//
+// A macro used to extract and assign an underlying value of type T
+// from an expression that returns an arrow::Result<T> to a variable.
+//
+// If the arrow::Status associated with the arrow::Result is "OK" (i.e. no error),
+// then the value of type T is assigned to the specified lhs.
+//
+// If the arrow::Status associated with the arrow::Result is not "OK" (i.e. error),
+// then the specified error ID is returned to MATLAB.
+//
+// **NOTE**: This macro should be used inside of the static make() member function for a
+// Proxy class. Use MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT inside of a non-static
+// Proxy member function.
+//
+// --- Arguments ---
+//
+// lhs - variable name to assign to (e.g. auto array)
+// rexpr - expression that returns an arrow::Result<T> (e.g. builder.Finish())
+// id - MATLAB error ID string (const char* - "arrow:matlab:proxy:make:FailedConstruction")
+//
+// --- Example ---
+//
+// MATLAB_ASSIGN_OR_ERROR(auto array, builder.Finish(), error::FAILED_TO_BUILD_ARRAY);
+//
+#define MATLAB_ASSIGN_OR_ERROR(lhs, rexpr, id) \
+ MATLAB_ASSIGN_OR_ERROR_IMPL(MATLAB_ASSIGN_OR_ERROR_NAME(_matlab_error_or_value, __COUNTER__), \
+ lhs, rexpr, id);
+
+
+#define MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT_IMPL(result_name, lhs, rexpr, context, id) \
+ auto&& result_name = (rexpr); \
+ MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(result_name.status(), context, id); \
+ lhs = std::move(result_name).ValueUnsafe();
+
+//
+// MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(lhs, rexpr, context, id)
+//
+// --- Description ---
+//
+// A macro used to extract and assign an underlying value of type T
+// from an expression that returns an arrow::Result<T> to a variable.
+//
+// If the arrow::Status associated with the arrow::Result is "OK" (i.e. no error),
+// then the value of type T is assigned to the specified lhs.
+//
+// If the arrow::Status associated with the arrow::Result is not "OK" (i.e. error),
+// then the specified error ID is returned to MATLAB.
+//
+// **NOTE**: This macro should be used inside of a non-static member function of a
+// Proxy class which has a libmexclass::proxy::method::Context as an input argument.
+// Use MATLAB_ASSIGN_OR_ERROR inside of a Proxy static make() member function.
+//
+// --- Arguments ---
+//
+// lhs - variable name to assign to (e.g. auto array)
+// rexpr - expression that returns an arrow::Result<T> (e.g. builder.Finish())
+// context - libmexclass::proxy::method::Context context input to a Proxy method
+// id - MATLAB error ID string (const char* - "arrow:matlab:proxy:make:FailedConstruction")
+//
+// --- Example ---
+//
+// MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(auto array, builder.Finish(), error::FAILED_TO_BUILD_ARRAY);
+//
+#define MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(lhs, rexpr, context, id) \
+ MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT_IMPL(MATLAB_ASSIGN_OR_ERROR_NAME(_matlab_error_or_value, __COUNTER__), \
+ lhs, rexpr, context, id);
+
namespace arrow::matlab::error {
// TODO: Make Error ID Enum class to avoid defining static constexpr
static const char* APPEND_VALUES_ERROR_ID = "arrow:matlab:proxy:make:FailedToAppendValues";
diff --git a/matlab/src/cpp/arrow/matlab/tabular/proxy/record_batch.cc b/matlab/src/cpp/arrow/matlab/tabular/proxy/record_batch.cc
index 27527599bb..c0b73833e5 100644
--- a/matlab/src/cpp/arrow/matlab/tabular/proxy/record_batch.cc
+++ b/matlab/src/cpp/arrow/matlab/tabular/proxy/record_batch.cc
@@ -33,15 +33,9 @@ namespace arrow::matlab::tabular::proxy {
void RecordBatch::toString(libmexclass::proxy::method::Context& context) {
namespace mda = ::matlab::data;
+ MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(const auto utf16_string, arrow::util::UTF8StringToUTF16(record_batch->ToString()), context, error::UNICODE_CONVERSION_ERROR_ID);
mda::ArrayFactory factory;
- const auto maybe_utf16_string = arrow::util::UTF8StringToUTF16(record_batch->ToString());
- // TODO: Add a helper macro to avoid having to write out an explicit if-statement here when handling errors.
- if (!maybe_utf16_string.ok()) {
- // TODO: This error message could probably be improved.
- context.error = libmexclass::error::Error{error::UNICODE_CONVERSION_ERROR_ID, maybe_utf16_string.status().message()};
- return;
- }
- auto str_mda = factory.createScalar(*maybe_utf16_string);
+ auto str_mda = factory.createScalar(utf16_string);
context.outputs[0] = str_mda;
}
@@ -63,18 +57,14 @@ namespace arrow::matlab::tabular::proxy {
std::vector<std::shared_ptr<Field>> fields;
for (size_t i = 0; i < arrow_arrays.size(); ++i) {
const auto type = arrow_arrays[i]->type();
- const auto column_name_str = std::u16string(column_names[i]);
- const auto maybe_column_name_str = arrow::util::UTF16StringToUTF8(column_name_str);
- MATLAB_ERROR_IF_NOT_OK(maybe_column_name_str.status(), error::UNICODE_CONVERSION_ERROR_ID);
- fields.push_back(std::make_shared<arrow::Field>(*maybe_column_name_str, type));
+ const auto column_name_utf16 = std::u16string(column_names[i]);
+ MATLAB_ASSIGN_OR_ERROR(const auto column_name_utf8, arrow::util::UTF16StringToUTF8(column_name_utf16), error::UNICODE_CONVERSION_ERROR_ID);
+ fields.push_back(std::make_shared<arrow::Field>(column_name_utf8, type));
}
arrow::SchemaBuilder schema_builder;
MATLAB_ERROR_IF_NOT_OK(schema_builder.AddFields(fields), error::SCHEMA_BUILDER_ADD_FIELDS_ERROR_ID);
- auto maybe_schema = schema_builder.Finish();
- MATLAB_ERROR_IF_NOT_OK(maybe_schema.status(), error::SCHEMA_BUILDER_FINISH_ERROR_ID);
-
- const auto schema = *maybe_schema;
+ MATLAB_ASSIGN_OR_ERROR(const auto schema, schema_builder.Finish(), error::SCHEMA_BUILDER_FINISH_ERROR_ID);
const auto num_rows = arrow_arrays.size() == 0 ? 0 : arrow_arrays[0]->length();
const auto record_batch = arrow::RecordBatch::Make(schema, num_rows, arrow_arrays);
auto record_batch_proxy = std::make_shared<arrow::matlab::tabular::proxy::RecordBatch>(record_batch);
@@ -98,14 +88,7 @@ namespace arrow::matlab::tabular::proxy {
std::vector<mda::MATLABString> column_names;
for (int i = 0; i < num_columns; ++i) {
const auto column_name_utf8 = record_batch->column_name(i);
- auto maybe_column_name_utf16 = arrow::util::UTF8StringToUTF16(column_name_utf8);
- // TODO: Add a helper macro to avoid having to write out an explicit if-statement here when handling errors.
- if (!maybe_column_name_utf16.ok()) {
- // TODO: This error message could probably be improved.
- context.error = libmexclass::error::Error{error::UNICODE_CONVERSION_ERROR_ID, maybe_column_name_utf16.status().message()};
- return;
- }
- auto column_name_utf16 = *maybe_column_name_utf16;
+ MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(auto column_name_utf16, arrow::util::UTF8StringToUTF16(column_name_utf8), context, error::UNICODE_CONVERSION_ERROR_ID);
const mda::MATLABString matlab_string = mda::MATLABString(std::move(column_name_utf16));
column_names.push_back(matlab_string);
}