zeroshade commented on PR #1113:
URL: https://github.com/apache/arrow-go/pull/1113#issuecomment-5221316979
Thanks for this — the architecture here is sound and worth keeping: the
checkpoint graph built once at construction, the parent-before-child restore
ordering, the multibuffer/string-view truncation, and the memo-table rehashing
all look correct. I found no refcount imbalance, no mutation of already-built
arrays, and no hash entry left reachable after truncation. Your test suite is
green, and `go vet`'s only complaint reproduces on unmodified `main`.
That said, I think this needs changes before merge. Everything below was
reproduced against `0b09f34c`.
## Root cause
The rollback path uses `Builder.Resize` as a transactional rewind primitive,
but `Resize` is an allocation/capacity hint, not an exact logical truncation —
it's free to round up, no-op, or compare in the wrong units, and it never
clears discarded validity bits. The fact that the PR already reaches into six
builders' unexported `length` fields to compensate is the design signalling
this. The four issues below are all the same cause.
## 1. `string` / `large_string` rollback silently corrupts data
Rows `{"s":"aaa","o":1}`, failing `{"s":"bbb","o":"bad"}`, then
`{"s":"ccc","o":2}` produces `["aaa", ""]` instead of `["aaa","ccc"]`.
`binarybuilder.go:252` compares a **byte** count against an **element**
count:
```go
b.offsets.resize((n + 1) * b.offsetByteWidth)
if (n * b.offsetByteWidth) < b.offsets.Len() { // bytes < elements
b.offsets.SetLength(n * b.offsetByteWidth) // never reached when
shrinking
}
```
`int32BufferBuilder.Len()` returns `b.length / arrow.Int32SizeBytes`
(elements), so shrinking 2→1 evaluates `4 < 2` → false and the offsets buffer
never shrinks. The stale offset leaves `offsets=[0,3,3,6]` with `length=2`, so
`value[1]` is `bytes[3:3]` = `""` while `"ccc"` sits unreferenced. It's silent
because a duplicated offset is a *legal* encoding of an empty string — no
panic, and `ValidateFull` can't catch it.
**Worth separating out:** this is a pre-existing latent bug, not a
regression from your diff. `RecordBuilder.Resize(-1)` (`record.go:370-381`,
from #805) already shrank builders mid-build, and reproduces the same
corruption with no JSON involved. I'd suggest fixing `binarybuilder.go:252` as
its own commit with a regression test — it's a real bug on `main` today. It
can't be deferred past this PR though, since this change makes the path
reachable from any JSON stream with a string column and one malformed row.
## 2. `FixedSizeBinaryBuilder` loses data
Rewinds `Len()` but never truncates its `values` byte buffer, and gets no
checkpoint (unlike `BinaryBuilder`). Same `aaa` / `bbb`(bad) / `ccc` sequence →
`["aaa","bbb"]`, with `"ccc"` lost.
## 3. `BooleanBuilder` / `NullBuilder` lengths are never rewound → panic
`NullBuilder.Resize` is a no-op (`null.go:141`); `BooleanBuilder.Resize`
rounds `n` up to `minBuilderCapacity` (32) before resizing
(`booleanbuilder.go:155-157`), so a failure within the first 32 values isn't
removed:
```
{a: bool, b: int32}, failed row {"a":true,"b":"invalid"}
after failed row: a.Len()=1 b.Len()=0
after next good row: a.Len()=2 b.Len()=1
NewRecordBatch panics: some fields have excessive number of rows (want at
most 1, have 2)
```
Identical with a leading `NullBuilder`. A recoverable decode error leaves
the builder permanently misaligned.
## 4. Discarded validity bits aren't cleared → rolled-back value resurfaces
Affects every field type. `resize` recomputes `nulls` for the kept prefix
but leaves the set bit behind, and `AppendNull` then increments `nulls` without
clearing it:
```
good {"a":10}, failed {"a":20,...}, then {"a":null,...}
array: [10 20] <- null was appended at index 1; the discarded 20 came
back
Len()=2 NullN()=1 IsNull(0)=false IsNull(1)=false
ValidateFull: null count value (1) does not match actual number of nulls in
array (0)
```
Nested variant: the list / list-view / fixed-size-list / map / struct
corrections restore `length` but not `nulls`, so after rollback `Len()==0`
while `NullN()==1`, poisoning the next row.
Since the goal of this PR is preventing failed rows from affecting
subsequent rows, it's worth noting the rollback path currently introduces that
same class of corruption for plain int32 and utf8 columns.
## Suggested fix shape
`Resize` can't carry this. I'd suggest a real per-builder exact-truncate
primitive (e.g. an unexported `truncate(n int)` on `Builder`), or extending
your existing `checkpointState` mechanism to *every* builder rather than only
Binary/BinaryView/dictionary — rewinding length, offsets, data buffers, and
children exactly, and zeroing the discarded validity-bitmap tail. `NullBuilder`
and `BooleanBuilder` need explicit length handling regardless, since their
`Resize` can't express a sub-32 truncation. This can't be fully fixed inside
`record.go`'s switch, because the state needing rewind is unexported
per-builder state that's unreachable for extension and third-party builders.
## Test gaps
Contract claims nine type families; several have no rollback coverage:
- **No rollback test at all:** maps, fixed-size lists, Boolean, Null,
large-list, large-list-view.
- **Length-only assertions** (`Len()==0`, no following-row content check):
sparse union, dense union, list-view.
- No failed-valid → null-at-same-index case, or failed-null → valid case
(these expose #4).
- No direct memo-table `Truncate` test (truncate → reinsert identical
dropped value, hash collisions, null indices).
- No test that builds an array, reuses the builder, rolls back, then asserts
the already-built array is unchanged.
- No custom `CheckpointableBuilder` test for repeated capture/restore or
checkpoint resource lifetime.
## Smaller notes
- `CheckpointState` has no commit/discard/release lifecycle — the REE
checkpoint can retain the previously decoded object after a successful batch,
and a custom checkpoint holding a refcounted resource has no success-path hook
to release it.
- The union child graph goes stale if `AppendChild` is called after
`RecordBuilder` construction; worth making that topology restriction explicit
or detected.
- `timestamp_with_offset.go`: every public `Resize`, including capacity
*growth*, resets `lastOffset`, forcing an unnecessary new REE run — correctness
is fine, but compression regresses.
- Doc comments for the exported `TimestampWithOffsetBuilder.Resize` and
`NewCheckpoint`; and the exported interfaces should document that one
checkpoint object is reused and `Capture` is called repeatedly.
Happy to help review the follow-up. I have runnable repro tests for all four
issues above if it'd be useful to have them posted 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]
