You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sgilmore10 (via GitHub)" <gi...@apache.org> on 2023/06/15 18:57:03 UTC
[GitHub] [arrow] sgilmore10 opened a new pull request, #36108: GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments
sgilmore10 opened a new pull request, #36108:
URL: https://github.com/apache/arrow/pull/36108
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou merged pull request #36108: GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #36108:
URL: https://github.com/apache/arrow/pull/36108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #36108: GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #36108:
URL: https://github.com/apache/arrow/pull/36108#issuecomment-1595942074
@austin3dickeyd Conbench reported that this pull request may have a performance regression but it should not be happen. Because we don't have MATLAB related benchmarks and this pull request changes only MATLAB related codes.
Can we reduce false-positive Conbench reports?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36108: GH-36098: [MATLAB] Change C++ proxy constructors to accept an options struct instead of a cell array containing the arguments
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36108:
URL: https://github.com/apache/arrow/pull/36108#issuecomment-1595896636
Conbench analyzed the 6 benchmark runs on commit `b041a428`.
There were 5 benchmark results indicating a performance regression:
- Commit Run on `arm64-t4g-linux-compute` at [2023-06-16 03:05:21Z](http://conbench.ursa.dev/compare/runs/dfb5ddae77c344a193ce99c5c7fb5241...63f35ead879e4f8386509b1524aa2bf9/)
- [params=simple_expression/rows_per_batch:1000000/real_time/threads:8, source=cpp-micro, suite=arrow-acero-expression-benchmark](http://conbench.ursa.dev/compare/benchmarks/0648bb5e8c4d7f058000131871bcdcc4...0648bd1bb2997d4f800086ae3c4d2a86)
- Commit Run on `arm64-m6g-linux-compute` at [2023-06-16 06:40:04Z](http://conbench.ursa.dev/compare/runs/5da8b814f04f45bebd115fe46466ea2c...b2f1de68b18e490fba9c8b15126ca519/)
- [params=<STATIC_VECTOR(std::string)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/0648bff73a3f7e668000dd95b10a3ab9...0648c0404e9e7272800004100edbd81f)
- and 3 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14347636241) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org