Skip to content

Commit

Permalink
GH-38630: [MATLAB] arrow.array.BooleanArray's toMATLAB method doe…
Browse files Browse the repository at this point in the history
…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>
  • Loading branch information
sgilmore10 committed Nov 9, 2023
1 parent 1f71014 commit 4983885
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 5 deletions.
2 changes: 1 addition & 1 deletion matlab/src/cpp/arrow/matlab/array/proxy/array.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ namespace arrow::matlab::array::proxy {
}

auto validity_bitmap = array->null_bitmap();
auto valid_elements_mda = bit::unpack(validity_bitmap, array_length);
auto valid_elements_mda = bit::unpack(validity_bitmap, array_length, array->offset());
context.outputs[0] = valid_elements_mda;
}

Expand Down
2 changes: 1 addition & 1 deletion matlab/src/cpp/arrow/matlab/array/proxy/boolean_array.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace arrow::matlab::array::proxy {
void BooleanArray::toMATLAB(libmexclass::proxy::method::Context& context) {
auto array_length = array->length();
auto packed_logical_data_buffer = std::static_pointer_cast<arrow::BooleanArray>(array)->values();
auto logical_array_mda = bit::unpack(packed_logical_data_buffer, array_length);
auto logical_array_mda = bit::unpack(packed_logical_data_buffer, array_length, array->offset());
context.outputs[0] = logical_array_mda;
}
}
3 changes: 1 addition & 2 deletions matlab/src/cpp/arrow/matlab/bit/unpack.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#include "arrow/util/bitmap_visit.h"

namespace arrow::matlab::bit {
::matlab::data::TypedArray<bool> unpack(const std::shared_ptr<arrow::Buffer>& packed_buffer, int64_t length) {
::matlab::data::TypedArray<bool> unpack(const std::shared_ptr<arrow::Buffer>& packed_buffer, int64_t length, int64_t start_offset) {
const auto packed_buffer_ptr = packed_buffer->data();

::matlab::data::ArrayFactory factory;
Expand All @@ -31,7 +31,6 @@ namespace arrow::matlab::bit {
auto unpacked_buffer_ptr = unpacked_buffer.get();
auto visitFcn = [&](const bool is_valid) { *unpacked_buffer_ptr++ = is_valid; };

const int64_t start_offset = 0;
arrow::internal::VisitBitsUnrolled(packed_buffer_ptr, start_offset, length, visitFcn);

::matlab::data::TypedArray<bool> unpacked_matlab_logical_Array = factory.createArrayFromBuffer({array_length, 1}, std::move(unpacked_buffer));
Expand Down
2 changes: 1 addition & 1 deletion matlab/src/cpp/arrow/matlab/bit/unpack.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@
#include "MatlabDataArray.hpp"

namespace arrow::matlab::bit {
::matlab::data::TypedArray<bool> unpack(const std::shared_ptr<arrow::Buffer>& packed_buffer, int64_t length);
::matlab::data::TypedArray<bool> unpack(const std::shared_ptr<arrow::Buffer>& packed_buffer, int64_t length, int64_t start_offset);
const uint8_t* extract_ptr(const ::matlab::data::TypedArray<bool>& unpacked_validity_bitmap);
}

0 comments on commit 4983885

Please sign in to comment.