GH-35914: [MATLAB] Integrate the latest libmexclass changes to suppor…

…t 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](mathworks/libmexclass@77f3d72) 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](mathworks/libmexclass@77f3d72). 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 <sgilmore@mathworks.com> Co-authored-by: sgilmore10 <74676073+sgilmore10@users.noreply.github.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
apache · Jun 8, 2023 · 4074233 · 4074233
1 parent 16328f0
commit 4074233
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git 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")