Conversation
… '\r' inside a quoted field With newlines_in_values, a quoted CRLF split across two input buffers was corrupted: CSVBufferIterator unconditionally skipped a leading '\n' whenever the previous buffer ended with '\r', so "xxx<CR>|<LF>yyy" inside a quoted field silently lost the '\n'. The iterator has no quoting context, so it cannot tell a CRLF line separator from quoted field contents. Move the straddling-CRLF decision into the block readers, where partial_ shows whether the previous '\r' was consumed as a line separator (partial empty -> skip the leading '\n') or is unfinished quoted content (keep it). The initial flag is plumbed through for the case where a header line ends exactly at a buffer boundary with '\r'. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
|
|
|
|
@Reranko05 Good catch — declared. Both the code and the PR description were written by an AI coding agent and reviewed by an AI agent before submission (running under my account). I've added the disclosure section to the PR description. Thanks! |
|
|
|
|
github-actions
Bot
added
awaiting committer review
Blocks without a complete row still yield a (possibly empty) chunk per block, so column(0)->chunk(0) does not necessarily hold the first row. Combine the table's chunks before indexing to keep the asserts valid for all three reader implementations. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Update on the earlier test failures: they were a bug in the new test, not in the reader changes.
The new commit calls |
|
@iliasabk We don't allow PRs from AI entirely, see https://arrow.apache.org/docs/developers/overview.html#ai-generated-code
|
3 participants
Rationale for this change
With
newlines_in_values = true, a quoted CRLF sequence split across two input buffers was silently corrupted:"…\rat the end of one buffer and\n…at the start of the next lost the\n(it stayed inside the field as\ronly). Reported in #51368.The root cause is in
CSVBufferIterator, which unconditionally skipped a leading\nwhenever the previous buffer ended with\r. At that layer there is no quoting context, so the code cannot distinguish a\r\nthat is a physical line separator from\r\nthat is the contents of an unfinished quoted field.What changes are included in this PR?
CSVBufferIteratorno longer resolves straddling CRLF sequences (it still strips a leading UTF-8 BOM).BlockReadernow owns the decision viatrailing_cr_+ aSkipStraddlingCRLFhelper: the next buffer's leading\nis skipped only when the previous buffer's trailing\rwas fully consumed as a line separator. When the\rbelongs to an unfinished quoted field, it is carried inpartial_(non-empty), the flag stays false, and the\nis preserved as field contents.SerialBlockReader(throughconsume_bytes, driven by how much of the buffer the parser consumed) andThreadedBlockReader(throughnext_partialfrom the chunker), including the row-skipping paths. Empty buffers never overwrite the state.prev_ended_crflag is plumbed through the block-reader factories for the edge case where a header row ends exactly at a buffer boundary with\r(the post-header buffer is then empty and the next buffer starts with\n).TestStraddlingCRLFcovering a quoted\r\nsplit across the boundary, a line-separator\r\nsplit across the boundary, and a header row ending with\rat the boundary. Registered for the serial, threaded-async and streaming readers.Are these changes tested?
Yes — a new
StraddlingCRLFtest incsv/reader_test.cccovers all three cases above with a block size that forces the split. (Note: the change was developed and cross-validated against a transliteration of the reader/chunker/parser pipeline; the gtest cases should be run in CI.)Are there any user-facing changes?
No API changes. Files with
newlines_in_valuesenabled whose quoted fields contain\r\nstraddling a read block boundary are now parsed correctly instead of losing the\n.Closes #51368
Was AI used for this PR?
PR code and description written by:
Reviewed before submission by: