fix(arrow/csv): append null for invalid fixed-size binary values - #1121
fix(arrow/csv): append null for invalid fixed-size binary values#1121fallintoplace wants to merge 3 commits into
Conversation
34c045c to
f9a159b
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The one-line fix in reader.go is right, and consistent with the rest of the file — every other parse function already does r.err = …; field.AppendNull(); return, and the base64-decode branch four lines above does exactly this. Good catch.
CI is red on all 15 test jobs though, and it's the new test rather than the fix — details inline. Once that's sorted this should be good to go.
While you're in here: the same missing-AppendNull defect exists in three sibling paths, if you'd like to fold them into this PR rather than opening more:
parseExtension—r.err = err; returnwith no appendparseListLike— theinvalid list formatbranch returns before appendingparseFixedSizeList— sameinvalid list formatbranch
And one that's worse than a ragged record: parseFixedSizeList's len(items) != n branch sets r.err after field.Append(true) has already opened a list slot, but appends no child values. A FixedSizeList requires its child to hold exactly n × parent_len entries, so that path yields a structurally invalid array, not just a misaligned one. Worth fixing regardless of what happens to this PR.
Note: this review was drafted with AI assistance by a maintainer and may contain mistakes. If anything here looks wrong, say so on the PR and I'll take another look.
| require.ErrorIs(t, r.Err(), arrow.ErrInvalid) | ||
|
|
||
| record := r.RecordBatch() | ||
| defer record.Release() |
There was a problem hiding this comment.
This double-releases the record batch, which is what's failing CI.
RecordBatch() returns r.cur directly — a borrowed reference, no Retain(). The reader owns it ("valid until the next call to Next"), and Reader.Release() releases r.cur itself. No other test in this file releases the batch it gets back from RecordBatch().
Defers run LIFO, so record.Release() fires first and drops the refcount to zero, then r.Release() releases it a second time and trips debug.Assert(rec.refCount.Load() > 0, "too many releases") in array/record.go:244. The stack lands on line 246, the closing brace, which is where the deferred calls execute.
CI runs with -tags assert,test,ccalloc, which is why this fires there but not under a plain go test ./arrow/csv — that's why the command in the PR description passes locally. Worth running with -tags assert when a test touches refcounts.
Fix: drop this line. (Or record.Retain() before it, but simply removing it matches the rest of the file.)
Rationale for this change
When a fixed-size binary value has the wrong decoded width, the CSV reader sets Err but does not append a value. Next then returns a record with no row for that field.
What changes are included in this PR?
Append a null after reporting the fixed-size binary length error so the record keeps the same number of rows as the other fields.
Are these changes tested?
go test ./arrow/csv -run TestFixedSizeBinaryParseErrorAppendsNullAre there any user-facing changes?
Parse failures now keep the row and expose a null value for the invalid field.