You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/08 15:34:41 UTC
[PR] fix(arrow/csv): append null for later parse failures [arrow-go]
fallintoplace opened a new pull request, #1149:
URL: https://github.com/apache/arrow-go/pull/1149
## What
CSV scalar parsers already append a null when their own parse fails, but several only did so while the reader had no earlier error. A malformed value in a later column could therefore append the parser zero value after another column failed. This keeps the first error and appends null for every failed scalar conversion.
## Test
- go test ./arrow/csv -run TestCSVReaderAppendsNullAfterPreviousParseError -count=1
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/csv): append null for later parse failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1149:
URL: https://github.com/apache/arrow-go/pull/1149#discussion_r3769531319
##########
arrow/csv/reader_test.go:
##########
@@ -901,6 +901,36 @@ func benchRead(b *testing.B, raw []byte, rows, cols, chunks int) {
}
}
+func TestCSVReaderAppendsNullAfterPreviousParseError(t *testing.T) {
+ schema := arrow.NewSchema([]arrow.Field{
Review Comment:
This regression covers the scalar converters updated here, but the same failure invariant is still broken for list and fixed-width converters. With an earlier `int8` parse error:
- malformed `List` panics while building the record
- wrong-length `FixedSizeList` panics
- wrong-width `FixedSizeBinary` panics
Those converters return without appending a value, leaving builder lengths inconsistent. Please extend the fix and regression coverage so every failed field conversion contributes one null rather than allowing `NewRecordBatch` to panic.
##########
arrow/csv/reader.go:
##########
@@ -385,6 +385,12 @@ func (r *Reader) isNull(val string) bool {
return false
}
+func (r *Reader) setParseError(err error) {
Review Comment:
Please route all conversion failures through this first-error helper, not only the numeric, temporal, and decimal subset. The binary converters below still assign `r.err` directly.
Reproduced with `first-error,%%%` under an `int8,binary` schema: the later base64 error replaces the original integer error. This contradicts the intended “keep the first error” behavior.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/csv): append null for later parse failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1149:
URL: https://github.com/apache/arrow-go/pull/1149
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org