Repository navigation
fix: Ensure ArrowArrayBuffer() and ArrowArraySetBuffer() work for variadic buffers - #808
Conversation
paleolimbot
changed the title
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Nice! I think this generally makes a lot of sense
| return private_data->buffers; | ||
| default: | ||
| return private_data->buffers + i - 1; | ||
| if (array->n_buffers > 3 && i == (array->n_buffers - 1)) { |
There was a problem hiding this comment.
What does this first condition do that isn't covered by the second?
There was a problem hiding this comment.
I'm going for "the last buffer of an array with variadic buffers"...I think if I didn't have the first condition it would pick up any last buffer (in any case it should be clearer what it's trying to do!)
| const bool is_binary_view = private_data->storage_type == NANOARROW_TYPE_STRING_VIEW || | ||
| private_data->storage_type == NANOARROW_TYPE_BINARY_VIEW; | ||
| const int32_t nfixed_buf = is_binary_view ? 2 : NANOARROW_MAX_FIXED_BUFFERS; | ||
| if (array->n_buffers > NANOARROW_MAX_FIXED_BUFFERS) { |
There was a problem hiding this comment.
Is this condition safe from the case where you have a string/binary view but still only 3 buffers in total that are being managed?
There was a problem hiding this comment.
I think so (but check me here because this logic is in a few places): an array has variadic buffers if and only if there are four or more buffers. In the case where a string/binary view has 3 buffers, the sizes buffer shouldn't be accessed/is probably NULL/0 sized in most cases.
There was a problem hiding this comment.
It has been some time but I think that the string/binary view implementations only have 2 fixed buffers, so its possible to have 3 buffers if you have only one variadic buffer:
arrow-nanoarrow/src/nanoarrow/common/array.c
Line 482 in f82379a
There was a problem hiding this comment.
I think that if you have one variadic buffer you would have four total (validity, data, variadic, variadic sizes) (this PR sort of conceptualizes the variadic sizes buffer as fixed because it is always present, even though its index in the buffer list changes).
There was a problem hiding this comment.
Ah OK - I think that makes sense. I totally forgot about the variadic sizes buffer, and was mistakenly assuming that there would just be a single variadic buffer holding the data (assuming not everything was inline)
22e3f4c to
88cdef0
Compare
|
Thanks for the review! I'll clean this up tonight/tomorrow and add proper C tests 🙂 |
|
If there are no objections I'll merge later today! |
The high-level goal of this PR is to fix a bug where the R package couldn't create a shallow copy of a string view array. The R and Python packages create shallow copies by borrowing buffers from the original and constructing an array using
ArrowArraySetBuffer(). BecauseArrowArraySetBuffer()errored for any buffer other than 0, 1, and 2, string view arrays caused cryptic errors when they hit certain places in the R package.After this PR,
ArrowArrayBuffer()andArrowArraySetBuffer()work for any buffer, variadic or no. This means it's possible to create string/binary view arrays by buffer, even if it's mostly something we just do internally.I'd hoped this would be a more straightforward fix, but the assumption that there will always be <=3 buffers ran fairly deep.