Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-15650: [MATLAB] Rename the MEX gateway function #12424

Closed
wants to merge 5 commits into from

Conversation

lafiona
Copy link
Contributor

@lafiona lafiona commented Feb 14, 2022

Overview

The MATLAB Interface to Arrow contains a MEX function named mexfcn to delegate MATLAB calls to C++ functions that implement the functionality.

When the interface is installed, the name of the MEX function, mexfcn, is on the global MATLAB Search Path. Because the previous name only captured that it is related to MEX functionality, it may cause conflicts with functions outside of the MATLAB Interface to Arrow.

Implementation

The following changes were made to address the potential naming conflict:

  • Rename the MEX function to call.
    • The name of the MEX file will be: call.<mex-extension>
  • Install the MEX function to the MATLAB package folder hierarchy: +arrow/+cpp within the installation directory.
    • Within MATLAB, the function is accessed by the following function signature: arrow.cpp.call('<cpp-function-name>', <cpp-function-argument-1>, ..., <cpp-function-argument-N>).
  • Install all dependencies of the MEX function to the same package folder hierarchy.
  • Updated MATLAB tests to use the new function name.

Testing

  • The change was qualified locally on Windows 10 (Ninja and Visual Studio), macOS 11.5 (Ninja and Xcode) for the following configurations:
    • Build both Arrow and GTest.
    • Use ARROW_HOME and GTEST_ROOT.
  • Additionally, the GitHub Actions CI passed for the change on Linux and macOS.

Notes

Thank you for working on this pull request with me, @kevingurney!

@github-actions
Copy link

@lafiona
Copy link
Contributor Author

lafiona commented Feb 14, 2022

@nickhaddady, do you have any feedback for this design change?

Copy link

@nickhaddady nickhaddady left a comment

Choose a reason for hiding this comment

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

This is great! Really like the change to use arrow.cpp.call as it seems to match what is going on in other language bindings for arrow. Think we do need to change the name of the binary mex file that gets built thought to avoid potential future symbol collisions.

matlab/CMakeLists.txt Show resolved Hide resolved
@lafiona lafiona changed the title ARROW-15650: [MATLAB] Rename the MEX gateway function ARROW-15650: [MATLAB] Rename the MEX gateway function [WIP] Feb 14, 2022
@lafiona lafiona changed the title ARROW-15650: [MATLAB] Rename the MEX gateway function [WIP] ARROW-15650: [MATLAB] Rename the MEX gateway function Feb 18, 2022
@kou
Copy link
Member

kou commented Mar 3, 2022

Can we merge this?

@lafiona
Copy link
Contributor Author

lafiona commented Mar 3, 2022

Can we merge this?

Yes, this is ready to merge. Thank you!

@kou kou closed this in 93b192c Mar 3, 2022
@kou
Copy link
Member

kou commented Mar 3, 2022

OK.
Merged.

@ursabot
Copy link

ursabot commented Mar 3, 2022

Benchmark runs are scheduled for baseline = e989fb3 and contender = 93b192c. 93b192c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@kevingurney kevingurney deleted the ARROW-15650 branch August 21, 2023 18:14
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.

5 participants