Skip to content

Conversation

@sgilmore10
Copy link
Member

@sgilmore10 sgilmore10 commented Jun 25, 2025

Rationale for this change

This is a follow up to #38357 (comment).

In the C++ code for the MATLAB interface, we frequently wrap/unwrap Arrow types, like arrow::Array, into/from a corresponding libmexclass Proxy objects. This currently takes several lines of code, leading to lots of code duplication and introduces room for subtle implementation errors.

It would be helpful to abstract away some of the steps involved in Proxy creation/management into a set of helper utilities that we can reuse in the C++ code for the MATLAB interface.

What changes are included in this PR?

  1. Added a new utility function that wraps a std::shared_ptr<arrow::Array> in a std::shared_ptr<arrow::matlab::array::proxy::Array>. Its function signature is
arrow::Result<std::shared_ptr<arrow::matlab::array::proxy::Array>> wrap(const std::shared_ptr<arrow::Array>& array);`
  1. Added a new utility function that wraps a std::shared_ptr<arrow::Array> in a std::shared_ptr<arrow::matlab::array::proxy::Array> and adds the proxy to the libmexclass::proxy::ProxyManager. Its function signature is
arrow::Result<std::shared_ptr<arrow::matlab::array::proxy::Array>> wrap_and_manage(const std::shared_ptr<arrow::Array>& array);
  1. Added a new utility function that wraps a std::shared_ptr<arrow::DataType> in a std::shared_ptr<arrow::matlab::type::proxy::Type>. Its function signature is
arrow::Result<std::shared_ptr<arrow::matlab::type::proxy::Type>> wrap(const std::shared_ptr<arrow::DataType>& type);
  1. Added a new utility function that wraps a std::shared_ptr<arrow::DataType> in a std::shared_ptr<arrow::matlab::type::proxy::DataType> and adds the proxy to the libmexclass::proxy::ProxyManager. It's function signature is
arrow::Result<std::shared_ptr<arrow::matlab::type::proxy::Type>> wrap_and_manage(const std::shared_ptr<arrow::DataType>& type);
  1. Deleted arrow::matlab::array::proxy::wrap and arrow::matlab::type::proxy::wrap functions.

Are these changes tested?

Tested via the existing MATLAB test classes.

Are there any user-facing changes?

No.

@sgilmore10 sgilmore10 marked this pull request as ready for review June 25, 2025 19:20
Copy link
Member

@kevingurney kevingurney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great improvement to the way we are working with Proxy objects! Thanks for working on this!

Co-authored-by: Kevin Gurney <kevin.p.gurney@gmail.com>
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 25, 2025
sgilmore10 and others added 2 commits June 25, 2025 16:49
Co-authored-by: Kevin Gurney <kevin.p.gurney@gmail.com>
Co-authored-by: Kevin Gurney <kevin.p.gurney@gmail.com>
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 25, 2025
Copy link
Member

@kevingurney kevingurney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 25, 2025
@sgilmore10 sgilmore10 merged commit c5fdc8e into apache:main Jun 26, 2025
9 checks passed
@sgilmore10 sgilmore10 removed the awaiting merge Awaiting merge label Jun 26, 2025
@sgilmore10 sgilmore10 deleted the GH-38369 branch June 26, 2025 14:44
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit c5fdc8e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants