Repository navigation
Conversation
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 31adcd2. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
There was a problem hiding this comment.
🟡 Changes recommended
CsvFileFormat::Equals() currently compares delimiter even when delimiter_string overrides it, which can produce incorrect inequality for semantically identical parse options.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the Arrow C++ CSV reader to support multi-character field delimiters by introducing a new ParseOptions::delimiter_string that overrides the existing single-character delimiter, and updating the parser/chunker accordingly.
Changes:
- Add
ParseOptions::delimiter_string(higher priority thandelimiter) and validate delimiter interactions with quoting/escaping/line endings. - Implement multi-character delimiter matching in both the CSV parser and the chunker (lexing boundary finder).
- Add unit tests covering multi-character delimiters across reader, parser, and chunker.
File summaries
| File | Description |
|---|---|
| cpp/src/arrow/dataset/file_csv.cc | Include delimiter_string in format equality; reject CSV writing when delimiter_string is set. |
| cpp/src/arrow/csv/options.h | Add ParseOptions::delimiter_string with override semantics. |
| cpp/src/arrow/csv/options.cc | Extend ParseOptions::Validate() for delimiter-string constraints (escaping/quoting/CRLF). |
| cpp/src/arrow/csv/lexing_internal.h | Add delimiter helpers and a streaming delimiter matcher; adjust bulk filter delimiter byte selection. |
| cpp/src/arrow/csv/chunker.cc | Support multi-character delimiter detection (for quote recognition) during chunking when needed. |
| cpp/src/arrow/csv/parser.cc | Add multi-character delimiter matching in the field parser state machine. |
| cpp/src/arrow/csv/reader_test.cc | Add end-to-end reader test for multi-character delimiters. |
| cpp/src/arrow/csv/parser_test.cc | Add parser tests for multi-character delimiters, delimiter-prefix-at-block-end, and single-char delimiter_string. |
| cpp/src/arrow/csv/chunker_test.cc | Add chunker test for multi-character delimiter behavior with partial buffers. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
github-actions
Bot
added
awaiting committer review
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core CSV parsing/chunking state machines and delimiter handling, which warrants final human review for subtle correctness/performance edge cases.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit a161eff. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
| Status ParseOptions::Validate() const { | ||
| if (ARROW_PREDICT_FALSE(delimiter == '\n' || delimiter == '\r')) { | ||
| return Status::Invalid("ParseOptions: delimiter cannot be \\r or \\n"); | ||
| // The chunker handles escapes before delimiter matching, so allowing the escape |
There was a problem hiding this comment.
Is it cleaner to consolidate the two cases like below?
std::string invalid_chars = "\r\n";
if (escaping) {
invalid_chars.push_back(escape_char);
}
const bool use_delimiter_string = !delimiter_string.empty();
const auto invalid_pos =
use_delimiter_string ? delimiter_string.find_first_of(invalid_chars)
: invalid_chars.find(delimiter);
if (invalid_pos != std::string::npos) {
return Status::Invalid("ParseOptions: delimiter contains an invalid character");
}
const char delimiter_first_byte =
use_delimiter_string ? delimiter_string.front() : delimiter;
if (quoting && delimiter_first_byte == quote_char) {
return Status::Invalid(
"ParseOptions: delimiter cannot start with the quote character");
}There was a problem hiding this comment.
However, by doing this, we lose the specific reason for the error.
| } | ||
| if (!parse_options.delimiter_string.empty()) { | ||
| return Status::NotImplemented( | ||
| "Writing CSV files with delimiter_string is not supported"); |
There was a problem hiding this comment.
Perhaps letting users know that it is non spec-compliant?
There was a problem hiding this comment.
Currently, even if delimiter_string is a compliant single-character comma, it will be rejected.
a161eff to
dbe45a9
Compare
| if (!parse_options.delimiter_string.empty()) { | ||
| return Status::NotImplemented( | ||
| "Writing CSV files with delimiter_string is not supported"); | ||
| } |
Rationale for this change
Now the type of
delimiterinParseOptionsischar, and we can only read csv files whose delimiter is one character. I think we can lift this restriction.What changes are included in this PR?
Added a higher-priority
delimiter_stringtoParseOptions, and added support for multi-character delimiters in both the chunker and the parser.Are these changes tested?
Yes.
Are there any user-facing changes?
Yes. A new option
delimiter_stringis added toParseOptions.