[SPARK-57929][CORE][SQL] Support Binary/List view types in ArrowColumnVector#57210
[SPARK-57929][CORE][SQL] Support Binary/List view types in ArrowColumnVector#57210robert3005 wants to merge 1 commit into
Conversation
|
Can't find anything in the logs about the failure, looks like github flake |
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Thank you @robert3005, cc @cloud-fan PTAL
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Review: 0 blocking, 1 non-blocking, 0 nits. Clean, well-scoped extension. I verified the manual view-buffer decode in StringViewAccessor byte-for-byte against Arrow's own BaseVariableWidthViewVector.getData (arrow-vector 19.0.0) — the length/bufferIndex/offset positions and the inline vs long branches all match; it's correct.
Suggestions (non-blocking)
ArrowColumnVector.java:560— see inline comment on the untested multi-buffer path.
| + BaseVariableWidthViewVector.PREFIX_WIDTH; | ||
| int bufferIndex = views.getInt(bufferIndexOffset); | ||
| int start = views.getInt(bufferIndexOffset + BaseVariableWidthViewVector.BUF_INDEX_WIDTH); | ||
| ArrowBuf dataBuffer = accessor.getDataBuffers().get(bufferIndex); |
There was a problem hiding this comment.
Non-blocking (test coverage): the long-value branch here indexes getDataBuffers().get(bufferIndex), but every long value in the string_view/binary_view/deserialization tests is small enough to land in a single variadic data buffer, so bufferIndex is always 0 — the multi-buffer path is never exercised. Risk is low since the decode matches Arrow's own getData, but a test with enough long values to spill into a second data buffer would close the gap.
3 participants
What changes were proposed in this pull request?
Add support for constructing ArrowColumnVector from String/BinaryView and ListView
Why are the changes needed?
Arrow in spec 1.4 has added these as more efficient way to represent list and string types in memory
Does this PR introduce any user-facing change?
No, this is internal developer api
How was this patch tested?
Added Tests
Was this patch authored or co-authored using generative AI tooling?
Yes, this pr was authored with use of Fable 5