zeroshade commented on code in PR #1133:
URL: https://github.com/apache/arrow-go/pull/1133#discussion_r3752496663
##########
arrow/compute/exec/span.go:
##########
@@ -586,6 +586,8 @@ func getNumBuffers(dt arrow.DataType) int {
// within a single block (the default 32KB allocation in the
// builder is sufficient for most use cases).
return 3
+ case arrow.LIST_VIEW, arrow.LARGE_LIST_VIEW:
+ return 3
Review Comment:
Keeping this as its own case rather than folding it into the `return 3`
above was the right call — but it's worth a line saying why, since the
neighbouring case returns the same number for an entirely unrelated reason.
The `BINARY_VIEW`/`STRING_VIEW` 3 is a *heuristic cap*: view header plus one
overflow data buffer, bounded by `ArraySpan`'s fixed buffer array. The
list-view 3 is *exact*. Without a note, a reader hitting these back to back
will reasonably assume the variadic caveat carries over.
```suggestion
case arrow.LIST_VIEW, arrow.LARGE_LIST_VIEW:
// validity + offsets + sizes. Unlike the view types above, this
// count is exact rather than a cap on variadic data buffers.
return 3
```
##########
arrow/compute/exec_internals_test.go:
##########
@@ -580,6 +581,24 @@ func (e *ExecSpanItrSuite) TestZeroLengthInput() {
checkArgs(input)
}
+func TestFillZeroLengthListView(t *testing.T) {
Review Comment:
This is worth keeping for the buffer-allocation behaviour, but it reaches
`getNumBuffers` indirectly, from a different package, when a direct test
already exists.
`arrow/compute/exec/span_test.go` has a table-driven test for
`ArraySpan.NumBuffers()` with rows like `{"binary", …, 3}` and `{"int32", …,
2}`. Two rows there would cover the actual change in its own package, in the
established idiom:
```go
{"list view", fields{Type: arrow.ListViewOf(arrow.PrimitiveTypes.Int32)}, 3},
{"large list view", fields{Type:
arrow.LargeListViewOf(arrow.PrimitiveTypes.Int32)}, 3},
```
That table is also where someone adding a new type will look for the count
contract, so a future ListView regression is far more likely to be caught there
than here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
