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 <[email protected]>
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 <[email protected]>
Co-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Sarah Gilmore <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sarah Gilomore <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
.../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);
}
