zeroshade commented on code in PR #1023:
URL: https://github.com/apache/arrow-go/pull/1023#discussion_r3660289158
##########
arrow/array/table.go:
##########
@@ -353,6 +353,11 @@ func (tr *TableReader) Next() bool {
for i := range chunks {
j := tr.slots[i]
chunk := tr.chunks[i].Chunk(j)
+ for chunk.Len() == 0 {
+ j++
+ tr.slots[i] = j
+ chunk = tr.chunks[i].Chunk(j)
+ }
Review Comment:
This loop is unbounded, and `Chunked.Chunk(i)` does a raw slice index with
no bounds check — so if it ever ran past the last chunk it would panic with
index-out-of-range rather than failing gracefully.
As written it is safe for well-formed tables, since `Next()` has already
established `tr.cur < tr.max` and `NewTable` enforces equal column lengths, so
there must be a non-empty chunk ahead. But that safety depends on an invariant
several layers away, and a malformed or hand-constructed `Chunked` (or a future
change to that invariant) turns this into a panic.
Could you bound it explicitly? Something like:
```go
for chunk.Len() == 0 && j+1 < tr.chunks[i].NumChunks() {
j++
tr.slots[i] = j
chunk = tr.chunks[i].Chunk(j)
}
```
That keeps the fix and makes the termination condition local and obvious.
--
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]
