zeroshade commented on code in PR #1113:
URL: https://github.com/apache/arrow-go/pull/1113#discussion_r3769635097
##########
arrow/array/record.go:
##########
@@ -316,6 +317,10 @@ func NewRecordBuilder(mem memory.Allocator, schema
*arrow.Schema) *RecordBuilder
for i := 0; i < schema.NumFields(); i++ {
b.fields[i] = NewBuilder(b.mem, schema.Field(i).Type)
}
+ b.checkpoints = make([]*builderCheckpoint, len(b.fields))
Review Comment:
Caching these checkpoints assumes the field-builder identities never change,
but `Fields()` returns the mutable backing slice. Existing callers can replace
a field builder:
```go
old := rb.Fields()[0]
rb.Fields()[0] = replacement
old.Release()
```
The checkpoint still references `old`; the next JSON row captures or
restores through the released builder and panics. If the caller retains `old`,
rollback silently targets stale state instead of the active replacement.
Please either rebuild/validate checkpoints when field identities change or
stop exposing a mutable field slice. This incompatibility is introduced by
caching the checkpoint graph.
##########
internal/hashing/xxh3_memo_table_types.go:
##########
@@ -190,6 +202,16 @@ func (t *Table[T]) Reset() {
t.nullIdx = KeyNotFound
}
+func (t *Table[T]) Truncate(size int) {
Review Comment:
Unlike `BinaryMemoTable.Truncate`, this always rebuilds the hash table even
when `size >= t.Size()`. Besides unnecessary rehashing for equal-size rollback,
a sufficiently large no-op truncation attempts to allocate from the requested
size and panics:
```go
table := hashing.NewMemoTable[int32](0)
table.GetOrInsert(int32(7))
table.Truncate(math.MaxInt) // makeslice: len out of range
```
Please add the same early-return guard used by `BinaryMemoTable.Truncate`:
```go
if size >= t.Size() {
return
}
```
--
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]
