Skip to content

Commit

Permalink
GH-36249: [MATLAB] Create a MATLAB_ASSIGN_OR_ERROR macro to mirror …
Browse files Browse the repository at this point in the history 
…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: #36190 (comment).
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)). This is different than how errors are returned  to MATLAB from within a `Proxy` static `make()` method.

### 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 <kgurney@mathworks.com>
Co-authored-by: Kevin Gurney <kevin.p.gurney@gmail.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sarah Gilomore <sgilmore@mathworks.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
@kevingurney @sgilmore10 @kou
4 people committed Jun 27, 2023
1 parent 2b9138c commit d5d98ae
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 40 deletions.
8 changes: 2 additions & 6 deletions matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
10 changes: 3 additions & 7 deletions matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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));
}
Expand Down
134 changes: 131 additions & 3 deletions matlab/src/cpp/arrow/matlab/error/error.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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); \
Expand All @@ -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";
Expand Down
31 changes: 7 additions & 24 deletions matlab/src/cpp/arrow/matlab/tabular/proxy/record_batch.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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);
Expand All @@ -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);
}
Expand Down

0 comments on commit d5d98ae

Please sign in to comment.