wgtmac commented on code in PR #51286:
URL: https://github.com/apache/arrow/pull/51286#discussion_r4080115412
##########
cpp/src/arrow/csv/options.cc:
##########
@@ -23,8 +23,39 @@ namespace csv {
ParseOptions ParseOptions::Defaults() { return ParseOptions(); }
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
Review Comment:
Is it cleaner to consolidate the two cases like below?
```cpp
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");
}
```
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -499,6 +500,10 @@ Result<std::shared_ptr<FileWriter>>
CsvFileFormat::MakeWriter(
if (!Equals(*options->format())) {
return Status::TypeError("Mismatching format/write options.");
}
+ if (!parse_options.delimiter_string.empty()) {
Review Comment:
The new Dataset behavior has no tests. Could you add one for format equality
with the same effective delimiter but different legacy `delimiter`?
##########
cpp/src/arrow/csv/options.cc:
##########
@@ -23,8 +23,39 @@ namespace csv {
ParseOptions ParseOptions::Defaults() { return ParseOptions(); }
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
+ // character in a delimiter could make it disagree with the parser.
+ if (escaping) {
Review Comment:
These are new (benign) behavior changes. I think we need to add some test
cases to verify that these malformed inputs are now explicitly rejected.
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -499,6 +500,10 @@ Result<std::shared_ptr<FileWriter>>
CsvFileFormat::MakeWriter(
if (!Equals(*options->format())) {
return Status::TypeError("Mismatching format/write options.");
}
+ if (!parse_options.delimiter_string.empty()) {
+ return Status::NotImplemented(
+ "Writing CSV files with delimiter_string is not supported");
Review Comment:
Perhaps letting users know that it is non spec-compliant?
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -268,6 +268,38 @@ TEST(BlockParser, Basics) {
}
}
+TEST(BlockParser, MultiDelimiter) {
+ auto options = ParseOptions::Defaults();
+ options.delimiter_string = "||";
+
+ BlockParser parser(options);
+ AssertParseFinal(parser,
+ Views({"name||message||score\n",
"alice||\"hello||world\"||42"}));
+ AssertColumnsEq(parser,
+ {{"name", "alice"}, {"message", "hello||world"}, {"score",
"42"}},
+ {{false, false}, {false, true}, {false, false}});
+}
+
+TEST(BlockParser, DelimiterPrefix) {
+ auto options = ParseOptions::Defaults();
+ options.delimiter_string = "||";
+
+ BlockParser parser(options, /*num_cols=*/2);
+ AssertParsePartial(parser, "a||b|", 0);
+ AssertParseFinal(parser, "a||b|");
+ AssertColumnsEq(parser, {{"a"}, {"b|"}});
+}
+
+TEST(BlockParser, SingleCharacterDelimiterString) {
Review Comment:
Can we add a few more parser cases here? The current tests cover the happy
path, but not simple edge cases like consecutive delimiters (`a||||b`), a
delimiter at the start or end of a row, `escaping=true`, or combinations with
`pad_short_rows`/`ignore_extra_columns`. These are small tests and exercise the
new `FieldStart`/`InField` branches.
##########
cpp/src/arrow/csv/chunker_test.cc:
##########
@@ -193,6 +193,27 @@ TEST_P(BaseChunkerTest, QuotingNewline) {
}
}
+TEST_P(BaseChunkerTest, MultiDelimiter) {
+ if (!options_.newlines_in_values) {
+ return;
+ }
+ options_.delimiter_string = "||";
+ MakeChunker();
+
+ auto partial = std::make_shared<Buffer>("name|");
Review Comment:
Can we also add a mismatch case here? For example, with `delimiter_string =
"||"`, `newlines_in_values = true`, and input `a|b||"c\n d"||e\n`, the single
`|` in `a|b` should stay literal, the later `||` should start a quoted field,
and the embedded newline should not end the row. This would cover fallback and
quote recognition together.
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -373,6 +373,7 @@ bool CsvFileFormat::Equals(const FileFormat& format) const {
checked_cast<const CsvFileFormat&>(format).parse_options;
return parse_options.delimiter == other_parse_options.delimiter &&
+ parse_options.delimiter_string ==
other_parse_options.delimiter_string &&
parse_options.quoting == other_parse_options.quoting &&
Review Comment:
I think this is worth doing to make it more user friendly.
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -59,6 +60,22 @@ Status MismatchingColumns(const InvalidRow& row) {
inline bool IsControlChar(uint8_t c) { return c < ' '; }
+enum class DelimiterMatch { NoMatch, Match, Incomplete };
Review Comment:
`Incomplete` -> `Partial`? Incomplete seems like failed to match.
--
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]
