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

GH-36560: [MATLAB] Remove the DeepCopy name-value pair from arrow.array.<Numeric>Array constructors #36561

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

sgilmore10
Copy link
Member

@sgilmore10 sgilmore10 commented Jul 7, 2023

Rationale for this change

We initially added the DeepCopy name-value pair to the numeric array class constructors for testing purposes. When DeepCopy=true, the proxy classes copy the data from the MATLAB array when creating the underlying std::shared_ptr<arrow::NumericArray<CType>>. By default, we don't make a copy and instead store the original array as a property named MatlabArray. Doing so keeps the backing memory of the arrow array alive and avoids a copy.

What changes are included in this PR?

DeepCopy is no longer accepted as a name-value pair by the constructors of the numeric array classes.

Are these changes tested?

No tests were needed.

Are there any user-facing changes?

This is technically a user facing change, but the MATLAB interface is still experimental and under active development. We don't expect anyone to be affected by this change.

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Thanks for opening a pull request!

If this is not a minor PR. 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 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}

See also:

@sgilmore10 sgilmore10 changed the title [GH-36560] Remove the DeepCopy name-value pair from arrow.array.<Numeric>Array constructors GH-36560: [MATLAB] Remove the DeepCopy name-value pair from arrow.array.<Numeric>Array constructors Jul 7, 2023
Copy link
Member

@kou kou 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 review Awaiting review labels Jul 8, 2023
@kevingurney
Copy link
Member

+1

@kevingurney
Copy link
Member

It looks like these changes are ready to be merged. I'll go ahead and merge them now.

@kevingurney kevingurney merged commit 5c4ba87 into apache:main Jul 10, 2023
11 checks passed
@kevingurney kevingurney removed the awaiting merge Awaiting merge label Jul 10, 2023
@kevingurney kevingurney deleted the GH-36560 branch July 10, 2023 15:58
@conbench-apache-arrow
Copy link

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

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.

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.

[MATLAB] Remove the DeepCopy name-value pair from arrow.array.<Numeric>Array constructors
3 participants