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/08 02:02:05 UTC
[arrow] branch main updated: GH-35914: [MATLAB] Integrate the latest libmexclass changes to support error-handling (#35918)
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 407423394c GH-35914: [MATLAB] Integrate the latest libmexclass changes to support error-handling (#35918)
407423394c is described below
commit 407423394cb988669a0b5716899810a924c0aa4c
Author: sgilmore10 <74...@users.noreply.github.com>
AuthorDate: Wed Jun 7 22:01:48 2023 -0400
GH-35914: [MATLAB] Integrate the latest libmexclass changes to support error-handling (#35918)
### Rationale for this change
This change integrates the latest version of `mathworks/libmexclass` into the MATLAB interface which enables throwing MATLAB errors.
The [77f3d72](https://github.com/mathworks/libmexclass/commit/77f3d72c22a9ddab7b54ba325d757c3e82e57987) in `libmexclass` introduced the following changes:
1. Added a new class called `libmexclass::error::Error`.
2. Added a new field called `error` on the `libmexclass::proxy::method::Context` which is a `std::optional<libmexclass::proxy::Error>`. By default, this is a `std::nullopt`. To make MATLAB throw an error, set this optional.
3. To support throwing errors at construction, `libmexclass` now requires all `Proxy` subclasses to define a static `make` method: `static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments)`. This workflow is similar to using an `arrow::Result<T>`object.
Examples of throwing errors in MATLAB can be found on lines [45](https://github.com/mathworks/libmexclass/blob/77f3d72c22a9ddab7b54ba325d757c3e82e57987/example/proxy/Car.cpp#L45) and [94](https://github.com/mathworks/libmexclass/blob/77f3d72c22a9ddab7b54ba325d757c3e82e57987/example/proxy/Car.cpp#L94) in [libmexclass/example/proxy/Car.cpp](https://github.com/mathworks/libmexclass/blob/main/example/proxy/Car.cpp)
### What changes are included in this PR?
1. Pulled in the latest version of `libmexclass`: [77f3d72](https://github.com/mathworks/libmexclass/commit/77f3d72c22a9ddab7b54ba325d757c3e82e57987).
2. Added `static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments)` to `arrow::matlab::proxy::NumericArray<Type>`.
3. Throw an error when trying to create an unknown proxy class.
### Are these changes tested?
1. Added a new test class called `tGateway.m` that verifies `libmexclass.proxy.gateway("Create", ...)` errors if given the name of an unknown proxy class.
### Are there any user-facing changes?
No.
* Closes: #35914
Lead-authored-by: Sarah Gilmore <sg...@mathworks.com>
Co-authored-by: sgilmore10 <74...@users.noreply.github.com>
Co-authored-by: Sutou Kouhei <ko...@cozmixng.org>
Co-authored-by: Kevin Gurney <kg...@mathworks.com>
Signed-off-by: Sutou Kouhei <ko...@clear-code.com>
---
matlab/src/cpp/arrow/matlab/array/proxy/array.cc | 2 +-
matlab/src/cpp/arrow/matlab/array/proxy/array.h | 2 +-
.../cpp/arrow/matlab/array/proxy/numeric_array.h | 41 +++++++++++++---------
matlab/src/cpp/arrow/matlab/error/error.h | 40 +++++++++++++++++++++
matlab/src/cpp/arrow/matlab/proxy/factory.cc | 8 ++---
matlab/src/cpp/arrow/matlab/proxy/factory.h | 2 +-
matlab/test/arrow/gateway/tGateway.m | 28 +++++++++++++++
matlab/tools/cmake/BuildMatlabArrowInterface.cmake | 6 ++--
8 files changed, 103 insertions(+), 26 deletions(-)
diff --git a/matlab/src/cpp/arrow/matlab/array/proxy/array.cc b/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
index fc1d66ae24..56115f5a37 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
@@ -21,7 +21,7 @@
namespace arrow::matlab::array::proxy {
- Array::Array(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
+ Array::Array() {
// Register Proxy methods.
REGISTER_METHOD(Array, toString);
diff --git a/matlab/src/cpp/arrow/matlab/array/proxy/array.h b/matlab/src/cpp/arrow/matlab/array/proxy/array.h
index 0a69f6fcad..859dc48571 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/array.h
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/array.h
@@ -25,7 +25,7 @@ namespace arrow::matlab::array::proxy {
class Array : public libmexclass::proxy::Proxy {
public:
- Array(const libmexclass::proxy::FunctionArguments& constructor_arguments);
+ Array();
virtual ~Array() {}
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 ad2242a755..019add312d 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
@@ -17,7 +17,6 @@
#pragma once
-
#include "arrow/array.h"
#include "arrow/array/data.h"
#include "arrow/array/util.h"
@@ -26,6 +25,7 @@
#include "arrow/type_traits.h"
#include "arrow/matlab/array/proxy/array.h"
+#include "arrow/matlab/error/error.h"
#include "arrow/matlab/bit/bit_pack_matlab_logical_array.h"
#include "libmexclass/proxy/Proxy.h"
@@ -42,8 +42,12 @@ const uint8_t* getUnpackedValidityBitmap(const ::matlab::data::TypedArray<bool>&
template<typename CType>
class NumericArray : public arrow::matlab::array::proxy::Array {
public:
- NumericArray(const libmexclass::proxy::FunctionArguments& constructor_arguments)
- : arrow::matlab::array::proxy::Array(constructor_arguments) {
+ NumericArray(const std::shared_ptr<arrow::Array> numeric_array)
+ : arrow::matlab::array::proxy::Array() {
+ array = numeric_array;
+ }
+
+ static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
using ArrowType = typename arrow::CTypeTraits<CType>::ArrowType;
using BuilderType = typename arrow::CTypeTraits<CType>::BuilderType;
@@ -64,15 +68,16 @@ class NumericArray : public arrow::matlab::array::proxy::Array {
auto unpacked_validity_bitmap = has_validity_bitmap ? getUnpackedValidityBitmap(constructor_arguments[2]) : nullptr;
BuilderType builder;
- auto st = builder.AppendValues(dt, numeric_mda.getNumberOfElements(), unpacked_validity_bitmap);
-
- // TODO: handle error case
- if (st.ok()) {
- auto maybe_array = builder.Finish();
- if (maybe_array.ok()) {
- array = *maybe_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);
+
+ return std::make_shared<arrow::matlab::array::proxy::NumericArray<CType>>(std::move(maybe_array).ValueUnsafe());
+
} else {
const auto data_type = arrow::CTypeTraits<CType>::type_singleton();
const auto length = static_cast<int64_t>(numeric_mda.getNumberOfElements()); // cast size_t to int64_t
@@ -81,11 +86,15 @@ 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 packed_validity_bitmap = has_validity_bitmap ? arrow::matlab::bit::bitPackMatlabLogicalArray(constructor_arguments[2]).ValueOrDie() : nullptr;
-
+ std::shared_ptr<arrow::Buffer> packed_validity_bitmap;
+ if (has_validity_bitmap) {
+ // Pack the validity bitmap values.
+ auto maybe_buffer = arrow::matlab::bit::bitPackMatlabLogicalArray(constructor_arguments[2]);
+ MATLAB_ERROR_IF_NOT_OK(maybe_buffer.status(), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
+ packed_validity_bitmap = std::move(maybe_buffer).ValueUnsafe();
+ }
auto array_data = arrow::ArrayData::Make(data_type, length, {packed_validity_bitmap, data_buffer});
- array = arrow::MakeArray(array_data);
+ 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
new file mode 100644
index 0000000000..d54d276735
--- /dev/null
+++ b/matlab/src/cpp/arrow/matlab/error/error.h
@@ -0,0 +1,40 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+
+#include "arrow/status.h"
+#include "libmexclass/error/Error.h"
+
+#include <string_view>
+
+#define MATLAB_ERROR_IF_NOT_OK(expr, id) \
+ do { \
+ arrow::Status _status = (expr); \
+ if (!_status.ok()) { \
+ return libmexclass::error::Error{(id), _status.message()}; \
+ } \
+ } while (0)
+
+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";
+ static const char* BUILD_ARRAY_ERROR_ID = "arrow:matlab:proxy:make:FailedToAppendValues";
+ static const char* BITPACK_VALIDITY_BITMAP_ERROR_ID = "arrow:matlab:proxy:make:FailedToBitPackValidityBitmap";
+ static const char* UNKNOWN_PROXY_ERROR_ID = "arrow:matlab:proxy:UnknownProxy";
+}
diff --git a/matlab/src/cpp/arrow/matlab/proxy/factory.cc b/matlab/src/cpp/arrow/matlab/proxy/factory.cc
index b7f86b9fcf..e159c0ea37 100644
--- a/matlab/src/cpp/arrow/matlab/proxy/factory.cc
+++ b/matlab/src/cpp/arrow/matlab/proxy/factory.cc
@@ -18,12 +18,12 @@
#include "arrow/matlab/array/proxy/numeric_array.h"
#include "factory.h"
-
+#include "arrow/matlab/error/error.h"
#include <iostream>
namespace arrow::matlab::proxy {
-std::shared_ptr<Proxy> Factory::make_proxy(const ClassName& class_name, const FunctionArguments& constructor_arguments) {
+libmexclass::proxy::MakeResult Factory::make_proxy(const ClassName& class_name, const FunctionArguments& constructor_arguments) {
// Register MATLAB Proxy classes with corresponding C++ Proxy classes.
REGISTER_PROXY(arrow.array.proxy.Float32Array, arrow::matlab::array::proxy::NumericArray<float>);
REGISTER_PROXY(arrow.array.proxy.Float64Array, arrow::matlab::array::proxy::NumericArray<double>);
@@ -38,9 +38,7 @@ std::shared_ptr<Proxy> Factory::make_proxy(const ClassName& class_name, const Fu
REGISTER_PROXY(arrow.array.proxy.Int32Array , arrow::matlab::array::proxy::NumericArray<int32_t>);
REGISTER_PROXY(arrow.array.proxy.Int64Array , arrow::matlab::array::proxy::NumericArray<int64_t>);
- // TODO: Decide what to do in the case that there isn't a Proxy match.
- std::cout << "Did not find a matching C++ proxy for: " + class_name << std::endl;
- return nullptr;
+ return libmexclass::error::Error{error::UNKNOWN_PROXY_ERROR_ID, "Did not find matching C++ proxy for " + class_name};
};
}
diff --git a/matlab/src/cpp/arrow/matlab/proxy/factory.h b/matlab/src/cpp/arrow/matlab/proxy/factory.h
index 6f68ff4ac9..fd41a1c10a 100644
--- a/matlab/src/cpp/arrow/matlab/proxy/factory.h
+++ b/matlab/src/cpp/arrow/matlab/proxy/factory.h
@@ -26,7 +26,7 @@ using namespace libmexclass::proxy;
class Factory : public libmexclass::proxy::Factory {
public:
Factory() { }
- virtual std::shared_ptr<Proxy> make_proxy(const ClassName& class_name, const FunctionArguments& constructor_arguments);
+ virtual libmexclass::proxy::MakeResult make_proxy(const ClassName& class_name, const FunctionArguments& constructor_arguments);
};
}
diff --git a/matlab/test/arrow/gateway/tGateway.m b/matlab/test/arrow/gateway/tGateway.m
new file mode 100644
index 0000000000..c2b9ef9d68
--- /dev/null
+++ b/matlab/test/arrow/gateway/tGateway.m
@@ -0,0 +1,28 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tGateway < matlab.unittest.TestCase
+ % Tests for libmexclass.proxy.gateway error conditions.
+
+ methods (Test)
+ function UnknownProxyError(testCase)
+ % Verify the gateway function errors if given the name of an
+ % unknown proxy class.
+ id = "arrow:matlab:proxy:UnknownProxy";
+ fcn = @()libmexclass.proxy.gateway("Create", "NotAProxyClass", {});
+ testCase.verifyError(fcn, id);
+ end
+ end
+end
\ No newline at end of file
diff --git a/matlab/tools/cmake/BuildMatlabArrowInterface.cmake b/matlab/tools/cmake/BuildMatlabArrowInterface.cmake
index 0dda3fb770..1f4ab05b06 100644
--- a/matlab/tools/cmake/BuildMatlabArrowInterface.cmake
+++ b/matlab/tools/cmake/BuildMatlabArrowInterface.cmake
@@ -24,7 +24,8 @@ set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_NAME libmexclass)
# libmexclass is accessible for CI without permission issues.
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_GIT_REPOSITORY "https://github.com/mathworks/libmexclass.git")
# Use a specific Git commit hash to avoid libmexclass version changing unexpectedly.
-set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_GIT_TAG "44c15d0")
+set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_GIT_TAG "77f3d72")
+
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_SOURCE_SUBDIR "libmexclass/cpp")
# ------------------------------------------
@@ -34,7 +35,8 @@ set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_FETCH_CONTENT_SOURCE_SUBDIR "libmexclass/cpp
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_PROXY_LIBRARY_NAME arrowproxy)
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_PROXY_LIBRARY_ROOT_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/src/cpp")
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_PROXY_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/array/proxy"
- "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/bit")
+ "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/bit"
+ "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/error")
set(MATLAB_ARROW_LIBMEXCLASS_CLIENT_PROXY_SOURCES "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/array/proxy/array.cc"
"${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/bit/bit_pack_matlab_logical_array.cc"
"${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/bit/bit_unpack_arrow_buffer.cc")