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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
