-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-37203: [MATLAB] Remove unused feather V1 MEX infrastructure and code #37204
Conversation
Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
cmake error 2. Remove commented out MEX-specific lines in CMakeLists.txt Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
(the old c-mex feather implementation) has been deleted.
Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sgilmore10! It's nice to see this old code being removed.
Since all of these changes are just deletions of unused code (aside from the one line change to move the definition of CMAKE_PACKAGED_INSTALL_DIR
), I think these changes can be safely merged as soon as the CI checks pass.
+1 |
…CE` flag from CMake build system and build new MATLAB Interface code by default (#37211) ### Rationale for this change Now that the old Feather V1 code and associated build infrastructure has been removed (#37204), it makes sense to start building the new, experimental MATLAB Interface code by default (without needing to [explicitly specify `-D MATLAB_ARROW_INTERFACE=ON`](https://github.com/apache/arrow/tree/main/matlab#build)). This pull request removes the `MATLAB_ARROW_INTERFACE` flag entirely, since setting it to `OFF` when we are building the MATLAB Interface code by default would essentially imply that no code should be built. ### What changes are included in this PR? 1. Removed mention of `MATLAB_ARROW_INTERFACE` flag from MATLAB `README.md`. 2. Removed conditional check for `MATLAB_ARROW_INTERFACE` flag from MATLAB `CMakeLists.txt`. 3. Removed `MATLAB_ARROW_INTERFACE` flag from `matlab_build.sh` CI script. ### Are these changes tested? Yes. The MATLAB Interface is building as expected by default on my Debian 11 machine. ### Are there any user-facing changes? Yes. 1. The experimental MATLAB Interface APIs will now be built by default without users explicitly specifying `-D MATLAB_ARROW_INTERFACE=ON` to `cmake`. 2. If users specify a value for `MATLAB_ARROW_INTERFACE`, the value will be ignored by the CMake build system. * Closes: #37209 Authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1aa5850. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them. |
…and code (apache#37204) ### Rationale for this change Now that `featherread` and `featherwrite` have been re-implemented in terms of the new MATLAB Interface APIs (apache#37163 and apache#37047), we can remove the unused feather V1 MEX infrastructure and code. ### What changes are included in this PR? 1. Deleted the following source and header files that are specific to the feather V1 MEX implementation: - `arrow/matlab/src/cpp/arrow/matlab/feather/feather_functions.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/feather/feather_reader.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/feather/feather_writer.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/feather/matlab_traits.h` - `arrow/matlab/src/cpp/arrow/matlab/feather/util/handle_status.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/feather/util/unicode_conversion.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/mex/call.cc` - `arrow/matlab/src/cpp/arrow/matlab/mex/mex_functions.h` - `arrow/matlab/src/cpp/arrow/matlab/mex/mex_util.[cc][h]` - `arrow/matlab/src/cpp/arrow/matlab/mex/mex_util_test.cc` - `arrow/matlab/src/cpp/arrow/matlab/api/visibility.h` 2. Deleted the following feather V1 MEX-specific build infrastructure files: - `arrow/matlab/build_support/common_vars.m` - `arrow/matlab/build_support/compile.m` - `arrow/matlab/build_support/test.m` 3. Removed all feather V1 MEX-specific logic from the `arrow/matlab/CMakeLists.txt` file. ### Are these changes tested? No tests are needed. The old feather V1 MEX specific implementation is unused code. ### Are there any user-facing changes? No. ### Future Directions 1. Review the back-log of stale tasks/issues that are no longer actionable and close them. For example, apache#27758 has already been implemented and submitted with a different issue attached. * Closes: apache#37203 Lead-authored-by: Sarah Gilmore <sgilmore@mathworks.com> Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
…NTERFACE` flag from CMake build system and build new MATLAB Interface code by default (apache#37211) ### Rationale for this change Now that the old Feather V1 code and associated build infrastructure has been removed (apache#37204), it makes sense to start building the new, experimental MATLAB Interface code by default (without needing to [explicitly specify `-D MATLAB_ARROW_INTERFACE=ON`](https://github.com/apache/arrow/tree/main/matlab#build)). This pull request removes the `MATLAB_ARROW_INTERFACE` flag entirely, since setting it to `OFF` when we are building the MATLAB Interface code by default would essentially imply that no code should be built. ### What changes are included in this PR? 1. Removed mention of `MATLAB_ARROW_INTERFACE` flag from MATLAB `README.md`. 2. Removed conditional check for `MATLAB_ARROW_INTERFACE` flag from MATLAB `CMakeLists.txt`. 3. Removed `MATLAB_ARROW_INTERFACE` flag from `matlab_build.sh` CI script. ### Are these changes tested? Yes. The MATLAB Interface is building as expected by default on my Debian 11 machine. ### Are there any user-facing changes? Yes. 1. The experimental MATLAB Interface APIs will now be built by default without users explicitly specifying `-D MATLAB_ARROW_INTERFACE=ON` to `cmake`. 2. If users specify a value for `MATLAB_ARROW_INTERFACE`, the value will be ignored by the CMake build system. * Closes: apache#37209 Authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
Now that
featherread
andfeatherwrite
have been re-implemented in terms of the new MATLAB Interface APIs (#37163 and #37047), we can remove the unused feather V1 MEX infrastructure and code.What changes are included in this PR?
Deleted the following source and header files that are specific to the feather V1 MEX implementation:
arrow/matlab/src/cpp/arrow/matlab/feather/feather_functions.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/feather/feather_reader.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/feather/feather_writer.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/feather/matlab_traits.h
arrow/matlab/src/cpp/arrow/matlab/feather/util/handle_status.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/feather/util/unicode_conversion.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/mex/call.cc
arrow/matlab/src/cpp/arrow/matlab/mex/mex_functions.h
arrow/matlab/src/cpp/arrow/matlab/mex/mex_util.[cc][h]
arrow/matlab/src/cpp/arrow/matlab/mex/mex_util_test.cc
arrow/matlab/src/cpp/arrow/matlab/api/visibility.h
Deleted the following feather V1 MEX-specific build infrastructure files:
arrow/matlab/build_support/common_vars.m
arrow/matlab/build_support/compile.m
arrow/matlab/build_support/test.m
Removed all feather V1 MEX-specific logic from the
arrow/matlab/CMakeLists.txt
file.Are these changes tested?
No tests are needed. The old feather V1 MEX specific implementation is unused code.
Are there any user-facing changes?
No.
Future Directions