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

MATLAB: Use arrow.internal.validate.index.numeric() in the column() method of arrow.tabular.RecordBatch #37155

Closed
sgilmore10 opened this issue Aug 14, 2023 · 1 comment · Fixed by #37156

Comments

@sgilmore10
Copy link
Member

Describe the enhancement requested

Now that the PR introducing the arrow.internal.validate.inex.numeric() function has been merged (#37150), we should use this utility function within the column() method of arrow.tabular.RecordBatch. Doing so will remove duplicate code from the code base.

Component(s)

MATLAB

@sgilmore10
Copy link
Member Author

take

kevingurney pushed a commit that referenced this issue Aug 14, 2023
…he `column()` method of `arrow.tabular.RecordBatch` (#37156)

### Rationale for this change

Now that the PR introducing the `arrow.internal.validate.index.numeric()` function has been merged (#37150), we should use this utility function within the `column()` method of `arrow.tabular.RecordBatch`. Doing so will remove duplicate code from the code base.

### What changes are included in this PR?

1. Updated the `column()` method of `arrow.tabular.RecordBatch` to use `arrow.internal.validate.index.numeric()`. 

### Are these changes tested?

Yes, these changes are covered by these two existing test classes:

1. `test/arrow/tabular/tRecordBatch.m`
2. `test/arrow/internal/validate/index/tNumeric.m`

In addition, I added a new test point to `tRecordBatch.m` called `ErrorIfNonPositiveIndex` to verify `arrow.internal.validate.index.numeric` is integrated correctly.

### Are there any user-facing changes?

No.

### Future Directions

1. In a future pull request, we will add support for indexing columns by name. Currently, the `column()` method only supports numeric indexing.
* Closes: #37155

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
@kevingurney kevingurney added this to the 14.0.0 milestone Aug 14, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…` in the `column()` method of `arrow.tabular.RecordBatch` (apache#37156)

### Rationale for this change

Now that the PR introducing the `arrow.internal.validate.index.numeric()` function has been merged (apache#37150), we should use this utility function within the `column()` method of `arrow.tabular.RecordBatch`. Doing so will remove duplicate code from the code base.

### What changes are included in this PR?

1. Updated the `column()` method of `arrow.tabular.RecordBatch` to use `arrow.internal.validate.index.numeric()`. 

### Are these changes tested?

Yes, these changes are covered by these two existing test classes:

1. `test/arrow/tabular/tRecordBatch.m`
2. `test/arrow/internal/validate/index/tNumeric.m`

In addition, I added a new test point to `tRecordBatch.m` called `ErrorIfNonPositiveIndex` to verify `arrow.internal.validate.index.numeric` is integrated correctly.

### Are there any user-facing changes?

No.

### Future Directions

1. In a future pull request, we will add support for indexing columns by name. Currently, the `column()` method only supports numeric indexing.
* Closes: apache#37155

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment