pitrou commented on code in PR #50846:
URL: https://github.com/apache/arrow/pull/50846#discussion_r3853147617
##########
cpp/src/arrow/csv/writer.cc:
##########
@@ -167,16 +167,32 @@ class ColumnPopulator {
// Copies the contents of s to out properly escaping any necessary characters.
// Returns the position next to last copied character.
-char* Escape(std::string_view s, char* out) {
+char* Escape(std::string_view s, char* out, EscapeStyle escape_style) {
for (const char c : s) {
+ if (c == '"' && escape_style == EscapeStyle::Backslash) {
+ *out++ = '\\';
+ }
*out++ = c;
- if (c == '"') {
+ if (c == '"' && escape_style == EscapeStyle::Double) {
*out++ = '"';
}
}
return out;
}
+// Returns the number of characters needed to escape the given string.
+int64_t EscapedLength(std::string_view s, EscapeStyle escape_style) {
+ int64_t quote_count = static_cast<int64_t>(std::count(s.begin(), s.end(),
'"'));
Review Comment:
Need to take `'\\'` characters as well.
##########
cpp/src/arrow/csv/options.h:
##########
@@ -174,6 +174,16 @@ struct ARROW_EXPORT ReadOptions {
Status Validate() const;
};
+/// \brief Escape style for CSV writing
+enum class ARROW_EXPORT EscapeStyle {
+ /// Quotes are escaped by doubling them, e.g. `""` (RFC4180 default)
+ Double,
Review Comment:
Call this `DoubleQuote` for better clarity?
##########
cpp/src/arrow/csv/writer_test.cc:
##########
Review Comment:
Can you add some backslashes in the input data?
##########
cpp/src/arrow/csv/writer_test.cc:
##########
@@ -306,6 +323,29 @@ std::vector<WriterTestParams> GenerateTestCases() {
/*delimiter=*/',', /*batch_size=*/5,
/*quoting_header=*/QuotingStyle::None),
"", expected_status_no_quotes_with_structural_in_header("b\"")},
+ // EscapeStyle::Backslash: quotes escaped by a preceding backslash.
+ {abc_schema, populated_batch,
+ DefaultTestOptions(/*include_header=*/false, /*null_string=*/"",
+ QuotingStyle::Needed, /*eol=*/"\n",
/*delimiter=*/',',
+ /*batch_size=*/5,
/*quoting_header=*/QuotingStyle::Needed,
+ /*escape_style=*/EscapeStyle::Backslash),
+ UtilGetExpectedWithEOLBackslash("\n")},
+ // EscapeStyle::Backslash with header (field name "b\"" escaped as
"b\\\"").
+ {abc_schema, populated_batch,
+ DefaultTestOptions(/*include_header=*/true, /*null_string=*/"",
+ QuotingStyle::Needed, /*eol=*/"\n",
/*delimiter=*/',',
+ /*batch_size=*/5,
/*quoting_header=*/QuotingStyle::Needed,
+ /*escape_style=*/EscapeStyle::Backslash),
+ R"("a","b\"","c ","d","e","f","g","h")"
+ "\n" +
+ UtilGetExpectedWithEOLBackslash("\n")},
+ // EscapeStyle::None: quotes are not escaped.
Review Comment:
This produces invalid CSV data, right?
##########
cpp/src/arrow/csv/writer.cc:
##########
@@ -167,16 +167,32 @@ class ColumnPopulator {
// Copies the contents of s to out properly escaping any necessary characters.
// Returns the position next to last copied character.
-char* Escape(std::string_view s, char* out) {
+char* Escape(std::string_view s, char* out, EscapeStyle escape_style) {
for (const char c : s) {
+ if (c == '"' && escape_style == EscapeStyle::Backslash) {
+ *out++ = '\\';
+ }
Review Comment:
What if c is `'\\'`? It needs to be escaped too.
##########
cpp/src/arrow/csv/writer.cc:
##########
@@ -167,16 +167,32 @@ class ColumnPopulator {
// Copies the contents of s to out properly escaping any necessary characters.
// Returns the position next to last copied character.
-char* Escape(std::string_view s, char* out) {
+char* Escape(std::string_view s, char* out, EscapeStyle escape_style) {
for (const char c : s) {
+ if (c == '"' && escape_style == EscapeStyle::Backslash) {
+ *out++ = '\\';
+ }
*out++ = c;
- if (c == '"') {
+ if (c == '"' && escape_style == EscapeStyle::Double) {
*out++ = '"';
}
}
return out;
}
+// Returns the number of characters needed to escape the given string.
+int64_t EscapedLength(std::string_view s, EscapeStyle escape_style) {
+ int64_t quote_count = static_cast<int64_t>(std::count(s.begin(), s.end(),
'"'));
+ switch (escape_style) {
+ case EscapeStyle::Double:
+ case EscapeStyle::Backslash:
+ return static_cast<int64_t>(s.length()) + quote_count;
+ case EscapeStyle::None:
+ return static_cast<int64_t>(s.length());
+ }
+ return static_cast<int64_t>(s.length());
Review Comment:
We're handling all enum values above so this shouldn't be necessary?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
