fix(arrow/array): avoid allocating for ReserveData(0)#895
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The ReserveData(0) short-circuit is reasonable, but the branch doesn't compile on its own:
arrow/array/binary_test.go:705:28: undefined: binaryViewNoAllocAllocator
FAIL github.com/apache/arrow-go/v18/arrow/array [build failed]
binaryViewNoAllocAllocator is introduced in #888 — it's not on main and not in this PR. Please rebase this on top of #888 (so the helper exists) or add the helper here. Once it builds, the guard + test look good.
Verified by checking out the PR head and running go test ./arrow/array/.
ecb3281 to
2c4b9b4
Compare
|
Rebased onto |
4b25336 to
7241331
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Rebase onto #888 resolves the earlier build failure — binaryViewNoAllocAllocator and checkBinaryViewValueSize now exist. Verified on the PR head (7241331):
go build ./arrow/array/clean;TestBinaryViewBuilderReserveData0DoesNotAllocate, the oversized-value guard, andTestBinaryViewStringRoundTripall pass../arrow/internal/arrjsonand the IPC variadic-count test pass.
The ReserveData(0) short-circuit is safe: inline values never touch the block builder, and any non-inline value reserves with a positive length before UnsafeAppend. Dropping the unused empty data block also correctly removes the "VARIADIC_DATA_BUFFERS": [""] entries — Array.Variadic is omitempty, the writer derives the count from len(Buffers())-2, the reader loads a dynamic count, and BinaryView.setData only requires >=2 buffers with dataBuffers = buffers[2:]. This also aligns Go with the C++ behavior of not emitting an empty variadic buffer for all-inline data.
LGTM.
Rationale for this change
BinaryViewBuilder.ReserveData always delegated to the block builder. ReserveData(0) could therefore create a full default data block even when every value remains inline.
What changes are included in this PR?
Are these changes tested?
Yes. A no-allocation allocator verifies that ReserveData(0) does not allocate. The array and arrjson packages pass normal and race tests.
Are there any user-facing changes?
Zero-length reservations no longer allocate an unused data block. Positive reservations and the public API are unchanged.