You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AdityRajGupta (via GitHub)" <gi...@apache.org> on 2026/10/17 14:32:45 UTC
[PR] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
AdityRajGupta opened a new pull request, #54059:
URL: https://github.com/apache/arrow/pull/54059
### Rationale for this change
With newlines_in_values=true, a \r\n inside a quoted field can be split across two read blocks. When \r is the last byte of one block and \n is the first byte of the next, the reader drops the \n and corrupts the field value. See #51368.
### What changes are included in this PR?
CSVBufferIterator now keeps a trailing \r and adds it to the next buffer instead of dropping the following \n. This keeps CRLF together across block boundaries. A pending \r at EOF is also preserved.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
**This PR contains a "Critical Fix"**: it fixes (b) a bug that caused incorrect data to be produced.
### Was AI used for this PR?
Yes AI was used for assistance during the development process mainly in understanding the issue.
<img width="980" height="488" alt="gh51368-new-tests" src="https://github.com/user-attachments/assets/2f2b8c4c-a8f3-4cfb-9d4d-dc0327df67da" />
<img width="980" height="664" alt="gh51368-full-suite" src="https://github.com/user-attachments/assets/43fcdd67-3a73-4828-9c2b-e691261201e9" />
Added regression tests for the serial, async, and streaming readers.
These particular test cases were failing before the fix. All 3 now pass with the fix.
**PR code and description written by:**
- [x] Human
- [x] AI
**Reviewed before submission by:**
- [x] Human
- [x] AI
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "dmitry-chirkov-dremio (via GitHub)" <gi...@apache.org>.
dmitry-chirkov-dremio commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4349980313
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
Review Comment:
With block_size=4, input "a,b\r1,2\r" has a complete header in the first block. Withholding its CR sends only "a,b" to ProcessHeader, which fails.
After fixing the move issue below, I get this failure in serial, async and streaming readers. The parent implementation reads the row correctly. Could we preserve this case and add regression coverage?
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "AdityRajGupta (via GitHub)" <gi...@apache.org>.
AdityRajGupta commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4351153777
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
Review Comment:
Withholding the first buffer's trailing '\r' left ProcessHeader without the header terminator. I now hold that first buffer and prepend it to the next one, so your exact block_size=4 example works. I added coverage for serial, async, and streaming readers, including CR and CRLF inputs at block sizes 4 and 64.
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
+ // Withhold a trailing '\r' so that downstream code never observes a
+ // buffer ending with '\r'. The chunker and the parser treat a '\r' at
+ // the end of a buffer as a complete line terminator; dropping the '\n'
+ // that may follow it (as was done previously) silently corrupts CRLF
+ // sequences inside quoted fields (GH-51368).
+ pending_cr_ = true;
+ buf = SliceBuffer(std::move(buf), offset, buf->size() - offset - 1);
Review Comment:
Thanks for the catch. Yes the slice length could be evaluated after buf was moved. I now calculate the slice length after moving buf and checked the function for the same pattern. The quoted CRLF boundary regression test is included, and the full CSV suite passed on Linux.
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "AdityRajGupta (via GitHub)" <gi...@apache.org>.
AdityRajGupta commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4351217508
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
+ // Withhold a trailing '\r' so that downstream code never observes a
+ // buffer ending with '\r'. The chunker and the parser treat a '\r' at
+ // the end of a buffer as a complete line terminator; dropping the '\n'
+ // that may follow it (as was done previously) silently corrupts CRLF
+ // sequences inside quoted fields (GH-51368).
+ pending_cr_ = true;
+ buf = SliceBuffer(std::move(buf), offset, buf->size() - offset - 1);
Review Comment:
Thanks for the catch. Yes, the slice length could be evaluated after buf was moved. I now calculate the slice length before moving buf, and checked the function for the same pattern. The quoted CRLF boundary regression test is included, and the full CSV suite passed on Linux.
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "AdityRajGupta (via GitHub)" <gi...@apache.org>.
AdityRajGupta commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4351153777
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
Review Comment:
Thanks for the catch. Yes the slice length could be evaluated after buf was moved. I now calculate the slice length after moving buf and checked the function for the same pattern. The quoted CRLF boundary regression test is included, and the full CSV suite passed on Linux.
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "dmitry-chirkov-dremio (via GitHub)" <gi...@apache.org>.
dmitry-chirkov-dremio commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4349974060
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
+ // Withhold a trailing '\r' so that downstream code never observes a
+ // buffer ending with '\r'. The chunker and the parser treat a '\r' at
+ // the end of a buffer as a complete line terminator; dropping the '\n'
+ // that may follow it (as was done previously) silently corrupts CRLF
+ // sequences inside quoted fields (GH-51368).
+ pending_cr_ = true;
+ buf = SliceBuffer(std::move(buf), offset, buf->size() - offset - 1);
Review Comment:
This can move buf before evaluating buf->size(), since SliceBuffer takes the buffer by value - QuotedCRLFStraddlingBlockBoundary crashes on my macOS ARM64 Release build. Could we calculate the length before moving buf?
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "AdityRajGupta (via GitHub)" <gi...@apache.org>.
AdityRajGupta commented on code in PR #54059:
URL: https://github.com/apache/arrow/pull/54059#discussion_r4351217508
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -120,25 +130,53 @@ class CSVBufferIterator {
first_buffer_ = false;
}
- if (trailing_cr_ && buf->data()[offset] == '\n') {
- // Skip '\r\n' line separator that started at the end of previous buffer
- ++offset;
+ if (pending_cr_) {
+ // Reattach the '\r' withheld from the end of the previous buffer to the
+ // start of this one. This keeps a CRLF pair straddling the boundary
+ // contiguous inside a single buffer, where both the chunker and the
+ // parser handle it correctly -- whether it is a line separator or data
+ // inside a quoted field (GH-51368).
+ pending_cr_ = false;
+ const int64_t rest = buf->size() - offset;
+ ARROW_ASSIGN_OR_RAISE(auto joined, AllocateBuffer(1 + rest));
+ uint8_t* out = joined->mutable_data();
+ out[0] = '\r';
+ if (rest > 0) {
+ memcpy(out + 1, buf->data() + offset, static_cast<size_t>(rest));
+ }
+ buf = std::shared_ptr<Buffer>(std::move(joined));
+ offset = 0;
+ }
+
+ if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') {
+ // Withhold a trailing '\r' so that downstream code never observes a
+ // buffer ending with '\r'. The chunker and the parser treat a '\r' at
+ // the end of a buffer as a complete line terminator; dropping the '\n'
+ // that may follow it (as was done previously) silently corrupts CRLF
+ // sequences inside quoted fields (GH-51368).
+ pending_cr_ = true;
+ buf = SliceBuffer(std::move(buf), offset, buf->size() - offset - 1);
Review Comment:
Withholding the first buffer's trailing '\r' left ProcessHeader without the header terminator. I now hold that first buffer and prepend it to the next one, so your exact block_size=4 example works. I added coverage for serial, async, and streaming readers, including CR and CRLF inputs at block sizes 4 and 64.
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #54059:
URL: https://github.com/apache/arrow/pull/54059#issuecomment-6235131350
:warning: GitHub issue #51368 **has been automatically assigned in GitHub** to PR creator.
--
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] GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… [arrow]
Posted by "AdityRajGupta (via GitHub)" <gi...@apache.org>.
AdityRajGupta commented on PR #54059:
URL: https://github.com/apache/arrow/pull/54059#issuecomment-6352804588
> dmitry-chirkov-dremio
yeahh you are totally right soo the fundamental problem was the iterator that had stripped off `\r` from the end of the block and appended it to the next one. If the read length was short enough, a resulting block did not have a terminator and got rejected by the chunker.
Now I have modified the iterator such that a block ending on `\r` is kept whole and appended to the next block in order to keep the stream consistent and fix the issue of block boundaries, not breaking anything in terms of existing cases of CRLF/header.
And I also added the requested coverage:
- Your `MixedLineEndingsStraddling` case across all 3 reader modes and multiple chunk splits.
- `SmallInputsAcrossBlockSizes`: 222 combinations covering CR/LF/CRLF, missing final terminators, quoted fields, embedded newlines, and block sizes 4–16.
The full CSV suite passes (292 tests). The matrix did find some cases that also fail on the parent implementation (rows spanning more than two blocks ), so those are documented and excluded. Everything else matches the parent, plus the GH-51368 quoted-CRLF corruption cases are now fixed :)
--
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