GH-30431: [C++] Add EscapeStyle to CSV Writer for configurable escape control - #50846
Open
waterWang wants to merge 1 commit into
Open
GH-30431: [C++] Add EscapeStyle to CSV Writer for configurable escape control#50846waterWang wants to merge 1 commit into
waterWang wants to merge 1 commit into
Conversation
…escape control Add EscapeStyle enum (Double, Backslash, None) to WriteOptions to control how quotes within quoted CSV values are escaped. Previously only double-quote escaping was supported (RFC4180). The new options are: - Double (default): quotes are escaped by doubling them, e.g. ```""``` - None: quotes are not escaped This aligns with readr::write_csv()'s escape argument. Also updates the header writing to use the same escape style for column names. [FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH]
|
|
kou
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the C++ CSV writer’s configurability by adding EscapeStyle to WriteOptions, allowing callers to choose how quotes inside quoted fields are escaped (double-quote, backslash, or none), aligning behavior with readr::write_csv()’s escape option.
Changes:
- Added
EscapeStyleenum andWriteOptions::escape_style(defaulting to RFC4180-style double-quote escaping). - Updated CSV writing logic (including header writing) to apply the selected escape style and to precompute escaped lengths for buffer sizing.
- Added tests covering backslash escaping (with/without header) and the “none” escape style.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpp/src/arrow/csv/writer.cc | Adds escape-style-aware escaping and escaped-length sizing; threads escape_style through populators and header writer. |
| cpp/src/arrow/csv/writer_test.cc | Extends writer tests to validate backslash and none escaping modes. |
| cpp/src/arrow/csv/options.h | Introduces the public EscapeStyle enum and the new WriteOptions::escape_style option. |
Suppressed comments (1)
cpp/src/arrow/csv/writer.cc:388
UpdateRowLengthscounts quotes twice per string (viaCountQuotesand again inEscapedLength), andEscapeStyle::Nonestill routes through the slowerEscape()path even though no escaping is performed. This is a measurable per-row performance regression for string-heavy writes; you can compute the escaped length directly from the first quote count and only markrow_needs_escaping_when escaping will actually happen.
// Each quote in the value string needs to be escaped.
int64_t escaped_count = CountQuotes(s);
row_needs_escaping_[row_number] = escaped_count > 0;
row_lengths[row_number] += EscapedLength(s, escape_style_) + kQuoteCount;
row_number++;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+183
to
+184
| // Returns the number of characters needed to escape the given string. | ||
| int64_t EscapedLength(std::string_view s, EscapeStyle escape_style) { |
kou
reviewed
Aug 12, 2026
kou
left a comment
kou
left a comment
Member
There was a problem hiding this comment.
Could you read https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code carefully?
Comment on lines
+172
to
178
| if (c == '"' && escape_style == EscapeStyle::Backslash) { | ||
| *out++ = '\\'; | ||
| } | ||
| *out++ = c; | ||
| if (c == '"') { | ||
| if (c == '"' && escape_style == EscapeStyle::Double) { | ||
| *out++ = '"'; | ||
| } |
Member
There was a problem hiding this comment.
Can we simplify this?
Suggested change
| if (c == '"' && escape_style == EscapeStyle::Backslash) { | |
| *out++ = '\\'; | |
| } | |
| *out++ = c; | |
| if (c == '"') { | |
| if (c == '"' && escape_style == EscapeStyle::Double) { | |
| *out++ = '"'; | |
| } | |
| if (c == '"') { | |
| if (escape_style == EscapeStyle::Double) { | |
| *out++ = '"'; | |
| } else if (escape_style == EscapeStyle::Backslash) { | |
| *out++ = '\\'; | |
| } | |
| } | |
| *out++ = c; | |
| } |
| row_needs_escaping_[row_number] = escaped_count > 0; | ||
| row_lengths[row_number] += | ||
| static_cast<int64_t>(s.length()) + escaped_count + kQuoteCount; | ||
| row_lengths[row_number] += EscapedLength(s, escape_style_) + kQuoteCount; |
| for (int col = 0; col < schema_->num_fields(); col++) { | ||
| const std::string& col_name = schema_->field(col)->name(); | ||
| header_length += col_name.size(); | ||
| header_length += EscapedLength(col_name, options_.escape_style); |
Member
There was a problem hiding this comment.
Can we call this when quoting_style == QuotingStyle::None?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Currently the CSV writer only supports double-quote escaping (RFC4180 default). This PR adds
EscapeStyletoWriteOptionsto support three escape modes, aligning withreadr::write_csv()'sescapeargument.What changes are included in this PR?
EscapeStyleenum inoptions.h:Double,Backslash,Noneescape_stylefield inWriteOptions(default:EscapeStyle::Doublefor backward compatibility)Escape()inwriter.ccto handle all three escape stylesEscapedLength()helper for accurate buffer size calculationescape_styleAre these changes tested?
Yes, added test cases:
EscapeStyle::Backslashwith and without headerEscapeStyle::None(quotes not escaped)Are there any user-facing changes?
Yes, users can now set
options.escape_styletoEscapeStyle::BackslashorEscapeStyle::Nonein addition to the defaultEscapeStyle::Double.