fix(arrow/csv): validate custom converter row counts - #1148
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LGTM — custom converter results are now validated before CSV matrix indexing, returning arrow.ErrInvalid for both short and long results without emitting partial rows.
I ran the regression 100 times, the focused race tests 20 times, a later-column atomicity probe, and the full arrow/csv package with its fixture submodules; all passed on head 95551a6d6600. CI is green.
One nonblocking documentation correction is noted inline: WithCustomTypeConverter should describe its result as one string per row/value rather than per CSV column.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.
| if w.customTypeConverter != nil { | ||
| result, handled := w.customTypeConverter(typ, col) | ||
| if handled { | ||
| if len(result) != col.Len() { |
There was a problem hiding this comment.
This correctly validates against col.Len(), but the public WithCustomTypeConverter documentation in common.go currently says the returned slice must match “the number of columns in the output csv.” Since this callback handles one column at a time, that should say it must contain one string per row/value in col. Could we update that documentation alongside the new validation?
2 participants
What
The custom CSV writer converter is documented to return one string per array value, but the writer trusted the result. Short results silently produced empty cells and long results could panic while filling the record matrix. This returns arrow.ErrInvalid for either mismatch.
Test