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.

Sign up for GitHub

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

Jump to bottom

[MATLAB] arrow.array.BooleanArray's toMATLAB method does not take slice offsets into account #38630

Closed
sgilmore10 opened this issue Nov 7, 2023 · 2 comments · Fixed by #38636
Closed

[MATLAB] arrow.array.BooleanArray's toMATLAB method does not take slice offsets into account #38630

sgilmore10 opened this issue Nov 7, 2023 · 2 comments · Fixed by #38636
Assignees
@sgilmore10
Labels
Component: MATLAB Type: bug
Milestone
15.0.0

Comments

@sgilmore10
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

While working on #38415, I noticed that the toMATLAB method of arrow.array.BooleanArray does not take the slice offset into account. This will cause the toMATLAB method to return the wrong value.

Component(s)

MATLAB
@sgilmore10
Copy link
Member Author

take

@sgilmore10
Copy link
Member Author

The same bug also manifests itself when accessing the Valid property on all sliced arrays. This is because both BooleanArray's toMATLAB method and the Valid getter function call arrow::matlab::bit::unpack(const std::shared_ptrarrow::Buffer& packed_buffer, int64_t length). This function calls VisitBitsUnrolled(const uint8_t* bitmap, int64_t start_offset, int64_t length, Visitor&& visit) with the assumption that start_offset is always 0. However, when the array has been sliced, start_offset will not be 0. Updating our unpack function to accept start_offset as input should fix both issues.

@github-actions github-actions bot mentioned this issue Nov 8, 2023
Merged
kevingurney pushed a commit that referenced this issue Nov 9, 2023
@sgilmore10
GH-38630: [MATLAB] arrow.array.BooleanArray's toMATLAB method doe…
4983885 
…s not take slice offsets into account (#38636)

### Rationale for this change

While working on #38415, I noticed that the `toMATLAB` method of `arrow.array.BooleanArray` does not take the slice offset into account. This will cause the `toMATLAB` method to return the wrong value.

### What changes are included in this PR?

1. Updated `arrow::matlab::bit::unpack` function to accept a `start_offset` as input.
2. Updated clients (`BooleanArray::toMATLAB` and `Array::getValid`) to supply the array `offset` as the `start_offset`.

### Are these changes tested?

The existing tests cover these changes. Additionally, the changes for #38415 will include tests that verify the `toMATLAB` method returns the correct MATLAB array when the underlying Arrow array has been sliced.

### Are there any user-facing changes?

No.

* Closes: #38630

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
@kevingurney kevingurney added this to the 15.0.0 milestone Nov 9, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
@sgilmore10 @loicalleyne
apacheGH-38630: [MATLAB] arrow.array.BooleanArray's toMATLAB meth…
bad2c66 
…od does not take slice offsets into account (apache#38636)

### Rationale for this change

While working on apache#38415, I noticed that the `toMATLAB` method of `arrow.array.BooleanArray` does not take the slice offset into account. This will cause the `toMATLAB` method to return the wrong value.

### What changes are included in this PR?

1. Updated `arrow::matlab::bit::unpack` function to accept a `start_offset` as input.
2. Updated clients (`BooleanArray::toMATLAB` and `Array::getValid`) to supply the array `offset` as the `start_offset`.

### Are these changes tested?

The existing tests cover these changes. Additionally, the changes for apache#38415 will include tests that verify the `toMATLAB` method returns the correct MATLAB array when the underlying Arrow array has been sliced.

### Are there any user-facing changes?

No.

* Closes: apache#38630

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@sgilmore10 @dgreiss
apacheGH-38630: [MATLAB] arrow.array.BooleanArray's toMATLAB meth…
fad365d 
…od does not take slice offsets into account (apache#38636)

### Rationale for this change

While working on apache#38415, I noticed that the `toMATLAB` method of `arrow.array.BooleanArray` does not take the slice offset into account. This will cause the `toMATLAB` method to return the wrong value.

### What changes are included in this PR?

1. Updated `arrow::matlab::bit::unpack` function to accept a `start_offset` as input.
2. Updated clients (`BooleanArray::toMATLAB` and `Array::getValid`) to supply the array `offset` as the `start_offset`.

### Are these changes tested?

The existing tests cover these changes. Additionally, the changes for apache#38415 will include tests that verify the `toMATLAB` method returns the correct MATLAB array when the underlying Arrow array has been sliced.

### Are there any user-facing changes?

No.

* Closes: apache#38630

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
Assignees

@sgilmore10 sgilmore10

Labels
Component: MATLAB Type: bug
Projects
Arrow MATLAB
Archived in project
Milestone
15.0.0
2 participants
@kevingurney @sgilmore10