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/15 20:52:11 UTC
[arrow] branch main updated: GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments (#36108)
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 b041a428a8 GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments (#36108)
b041a428a8 is described below
commit b041a428a890f28d70e0e04f471225ff131c9f81
Author: sgilmore10 <74...@users.noreply.github.com>
AuthorDate: Thu Jun 15 16:52:04 2023 -0400
GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments (#36108)
### Rationale for this change
It would be better if we passed an option struct to the C++ proxy constructors instead of a cell array containing the input arguments. If we pass in a struct, we can access the inputs by name. For example, if the NumericArray proxy class accepted an options struct with fields named `MatlabArray`, `Valid`, and `DeepCopy`, we could access the values like so in its `make` function:
```cpp
static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
::matlab::data::StructArray opts = constructor_arguments[0];
const ::matlab::data::TypedArray<CType> data_mda = opts[0]["MatlabArray"];
const ::matlab::data::TypedArray<bool> valid_mda = opts[0]["Valid"];
const ::matlab::data::TypedArray<bool> make_copy_mda = opts[0]["DeepCopy"];
}
```
It's easier to reason about the code above than the code snippet below:
```cpp
static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
const ::matlab::data::TypedArray<CType> data_mda = constructor_arguments[0];
const ::matlab::data::TypedArray<bool> valid_mda = constructor_arguments[1];
const ::matlab::data::TypedArray<bool> make_copy_mda = constructor_arguments[2];
}
```
Using options structs also enables us to support syntaxes at construction-time. We can query a field on the struct to determine which fields we should expect to be there.
### What changes are included in this PR?
1. The `NumericArray` C++ proxy classes accepts a struct with the fields `MatlabArray`, `Valid`, and `DeepCopy` at construction time.
2. The `BooleanArray` C++ proxy class accepts a struct with the fields `MatlabArray` and `Valid` at construction-time.
### Are these changes tested?
Existing test cases cover these changes.
### Are there any user-facing changes?
No, these changes are not user-facing.
### Future Directions
In a followup PR, we plan on adding an `ArgumentParser` class. This would abstract out how you access fields on scalar `matlab::data::StructArray` objects. The `NumericArray` code would then look something like this:
```cpp
static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
ArgumentParser args{constructor_arguments[0]};
const ::matlab::data::TypedArray<CType> data_mda = args["MatlabArray"];
const ::matlab::data::TypedArray<bool> valid_mda = args["Valid"];
const ::matlab::data::TypedArray<bool> make_copy_mda = args["DeepCopy"];
}
```
### Notes
Thank you to @ kevingurney for all the help and the idea for the `ArgumentParser` class.
* Closes: #36098
Authored-by: Sarah Gilmore <sg...@mathworks.com>
Signed-off-by: Sutou Kouhei <ko...@clear-code.com>
---
.../cpp/arrow/matlab/array/proxy/boolean_array.cc | 6 +++--
.../cpp/arrow/matlab/array/proxy/numeric_array.h | 28 ++++++++++------------
matlab/src/matlab/+arrow/+array/BooleanArray.m | 3 ++-
matlab/src/matlab/+arrow/+array/NumericArray.m | 3 ++-
4 files changed, 21 insertions(+), 19 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 def8a53e80..a3327fe703 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
@@ -24,9 +24,11 @@
namespace arrow::matlab::array::proxy {
libmexclass::proxy::MakeResult BooleanArray::make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
+ ::matlab::data::StructArray opts = constructor_arguments[0];
+
// Get the mxArray from constructor arguments
- const ::matlab::data::TypedArray<bool> logical_mda = constructor_arguments[0];
- const ::matlab::data::TypedArray<bool> validity_bitmap_mda = constructor_arguments[1];
+ const ::matlab::data::TypedArray<bool> logical_mda = opts[0]["MatlabArray"];
+ const ::matlab::data::TypedArray<bool> validity_bitmap_mda = opts[0]["Valid"];
// Pack the logical data values.
auto maybe_packed_logical_buffer = arrow::matlab::bit::bitPackMatlabLogicalArray(logical_mda);
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 019add312d..c4c439bff0 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/numeric_array.h
@@ -51,12 +51,13 @@ class NumericArray : public arrow::matlab::array::proxy::Array {
using ArrowType = typename arrow::CTypeTraits<CType>::ArrowType;
using BuilderType = typename arrow::CTypeTraits<CType>::BuilderType;
- // Get the mxArray from constructor arguments
- const ::matlab::data::TypedArray<CType> numeric_mda = constructor_arguments[0];
- const ::matlab::data::TypedArray<bool> make_copy = constructor_arguments[1];
-
- const auto has_validity_bitmap = constructor_arguments.getNumberOfElements() > 2;
+ ::matlab::data::StructArray opts = constructor_arguments[0];
+ // Get the mxArray from constructor arguments
+ const ::matlab::data::TypedArray<CType> numeric_mda = opts[0]["MatlabArray"];
+ const ::matlab::data::TypedArray<bool> valid_mda = opts[0]["Valid"];
+ const ::matlab::data::TypedArray<bool> make_copy = opts[0]["DeepCopy"];
+
// Get raw pointer of mxArray
auto it(numeric_mda.cbegin());
auto dt = it.operator->();
@@ -65,18 +66,17 @@ class NumericArray : public arrow::matlab::array::proxy::Array {
if (make_deep_copy) {
// Get the unpacked validity bitmap (if it exists)
- auto unpacked_validity_bitmap = has_validity_bitmap ? getUnpackedValidityBitmap(constructor_arguments[2]) : nullptr;
+ auto unpacked_validity_bitmap = getUnpackedValidityBitmap(valid_mda);
BuilderType builder;
-
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());
+ return std::make_shared<arrow::matlab::array::proxy::NumericArray<CType>>(*maybe_array);
} else {
const auto data_type = arrow::CTypeTraits<CType>::type_singleton();
@@ -86,13 +86,11 @@ 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());
- 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();
- }
+ // Pack the validity bitmap values.
+ auto maybe_buffer = arrow::matlab::bit::bitPackMatlabLogicalArray(valid_mda);
+ MATLAB_ERROR_IF_NOT_OK(maybe_buffer.status(), error::BITPACK_VALIDITY_BITMAP_ERROR_ID);
+ auto packed_validity_bitmap = *maybe_buffer;
+
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/matlab/+arrow/+array/BooleanArray.m b/matlab/src/matlab/+arrow/+array/BooleanArray.m
index 52fae56e2d..f4d341efce 100644
--- a/matlab/src/matlab/+arrow/+array/BooleanArray.m
+++ b/matlab/src/matlab/+arrow/+array/BooleanArray.m
@@ -29,7 +29,8 @@ classdef BooleanArray < arrow.array.Array
end
arrow.args.validateTypeAndShape(data, "logical");
validElements = arrow.args.parseValidElements(data, opts);
- obj@arrow.array.Array("Name", "arrow.array.proxy.BooleanArray", "ConstructorArguments", {data, validElements});
+ opts = struct(MatlabArray=data, Valid=validElements);
+ obj@arrow.array.Array("Name", "arrow.array.proxy.BooleanArray", "ConstructorArguments", {opts});
end
function data = logical(obj)
diff --git a/matlab/src/matlab/+arrow/+array/NumericArray.m b/matlab/src/matlab/+arrow/+array/NumericArray.m
index 86fae54ae1..16b96be7ac 100644
--- a/matlab/src/matlab/+arrow/+array/NumericArray.m
+++ b/matlab/src/matlab/+arrow/+array/NumericArray.m
@@ -37,7 +37,8 @@ classdef NumericArray < arrow.array.Array
end
arrow.args.validateTypeAndShape(data, type);
validElements = arrow.args.parseValidElements(data, nullOpts);
- obj@arrow.array.Array("Name", proxyName, "ConstructorArguments", {data, opts.DeepCopy, validElements});
+ opts = struct(MatlabArray=data, Valid=validElements, DeepCopy=opts.DeepCopy);
+ obj@arrow.array.Array("Name", proxyName, "ConstructorArguments", {opts});
obj.MatlabArray = cast(obj.MatlabArray, type);
% Store a reference to the array if not doing a deep copy
if ~opts.DeepCopy, obj.MatlabArray = data; end