Skip to content

fix(arrow/array): roll back JSON builder state after failed rows - #1113

Open
fallintoplace wants to merge 10 commits into
apache:mainfrom
fallintoplace:fix/array-json-row-rollback
Open

fix(arrow/array): roll back JSON builder state after failed rows#1113
fallintoplace wants to merge 10 commits into
apache:mainfrom
fallintoplace:fix/array-json-row-rollback

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

RecordBuilder mutates field builders while decoding a JSON object. If a later field fails, values appended for earlier fields remain and can affect subsequent rows.

What changes are included in this PR?

Build the nested checkpoint graph once when the RecordBuilder is created. Each row captures reusable lengths and builder state, then restores that state on decode errors. This covers lists, structs, maps, fixed-size lists, unions, dictionaries, variable-width buffers, string views, and run-end encoded children without rebuilding checkpoint trees for every successful row.

Parent builders are restored before their children so parent Resize calls cannot overwrite restored child state. This is especially important for nested run-end encoded builders.

Custom builders with internal state can participate through the exported CheckpointState and CheckpointableBuilder interfaces.

Are these changes tested?

  • go test ./arrow/array
  • go test ./arrow/extensions
  • go test ./arrow/array -run ^$ -bench BenchmarkRecordFromJSON/Size_1000$ -benchtime=1x -benchmem

Are there any user-facing changes?

Yes. Failed JSON rows are rolled back completely, including nested builder state. This also adds two exported interfaces for custom builders that need to restore internal state during row rollback.

@fallintoplace fallintoplace changed the title fix(arrow/array): roll back failed JSON rows fix(arrow/array): restore builders after failed JSON rows Aug 5, 2026
@fallintoplace fallintoplace changed the title fix(arrow/array): restore builders after failed JSON rows fix(arrow/array): roll back JSON builder state after failed rows Aug 6, 2026
@zeroshade

Copy link
Copy Markdown
Member

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:

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 Update package name #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.

@fallintoplace
fallintoplace force-pushed the fix/array-json-row-rollback branch from 0b09f34 to 724199f Compare August 7, 2026 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants