pitrou commented on code in PR #51286:
URL: https://github.com/apache/arrow/pull/51286#discussion_r4311815884
##########
cpp/src/arrow/csv/chunker.cc:
##########
@@ -262,6 +290,21 @@ class Lexer {
const ParseOptions& options_;
const BulkFilterType bulk_filter_;
State state_ = FIELD_START;
+
+ private:
Review Comment:
The private is a bit gratuitous here, no?
##########
cpp/src/arrow/csv/chunker.cc:
##########
@@ -380,23 +423,17 @@ std::unique_ptr<Chunker> MakeChunker(const ParseOptions&
options) {
if (!options.newlines_in_values) {
delimiter = MakeNewlineBoundaryFinder();
} else {
- if (options.quoting) {
- if (options.escaping) {
- delimiter = std::make_shared<
- LexingBoundaryFinder<internal::SpecializedOptions<true,
true>>>(options);
- } else {
- delimiter = std::make_shared<
- LexingBoundaryFinder<internal::SpecializedOptions<true,
false>>>(options);
- }
- } else {
- if (options.escaping) {
- delimiter = std::make_shared<
- LexingBoundaryFinder<internal::SpecializedOptions<false,
true>>>(options);
- } else {
- delimiter = std::make_shared<
- LexingBoundaryFinder<internal::SpecializedOptions<false,
false>>>(options);
- }
- }
+ // Unlike the parser, the chunker only needs delimiters to identify field
starts
+ // for quote recognition. Without quoting, delimiters cannot affect line
boundaries.
+ const bool multi_delimiter = options.quoting &&
!options.delimiter_string.empty();
Review Comment:
Should we actually check for `delimiter_string.length() > 1`?
##########
cpp/src/arrow/csv/options.h:
##########
@@ -65,6 +65,8 @@ struct ARROW_EXPORT ParseOptions {
bool pad_short_rows = false;
/// Whether rows with more columns than expected should ignore the extra
columns.
bool ignore_extra_columns = false;
+ /// Multi-character field delimiter. If non-empty, overrides `delimiter`.
+ std::string delimiter_string;
Review Comment:
I wonder if we can just make `delimiter` a `std::variant<char, std::string>`
or if that would be less ergonomic.
If we keep the current two-field approach, then perhaps we can mark
`delimiter` as deprecated?
##########
cpp/src/arrow/csv/chunker.cc:
##########
@@ -141,6 +162,9 @@ class Lexer {
}
c = *data++;
if (SpecializedOptions::escaping && ARROW_PREDICT_FALSE(c ==
options_.escape_char)) {
+ if constexpr (SpecializedOptions::multi_delimiter) {
+ delimiter_matcher_.Reset();
+ }
Review Comment:
Can the `Reset` call be done just at the start of `FieldStart` instead of in
multiple places like this?
##########
cpp/src/arrow/csv/chunker_test.cc:
##########
@@ -193,6 +193,47 @@ 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|");
+ auto block =
std::make_shared<Buffer>("|message||\"hello\nworld\"||42\nnext||row\n");
Review Comment:
Can we add more single pipes to exercise the delimiter search logic?
##########
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) {
+ if (delimiter_string.empty()) {
+ if (delimiter == escape_char) {
+ return Status::Invalid(
+ "ParseOptions: delimiter cannot be the escape character when
escaping is "
+ "enabled");
+ }
+ } else if (delimiter_string.find(escape_char) != std::string::npos) {
+ return Status::Invalid(
+ "ParseOptions: delimiter_string cannot contain the escape character
when "
+ "escaping is enabled");
+ }
+ }
+ // Line endings delimit records and therefore cannot be part of a field
delimiter.
+ if (delimiter_string.empty()) {
+ if (ARROW_PREDICT_FALSE(delimiter == '\n' || delimiter == '\r')) {
+ return Status::Invalid("ParseOptions: delimiter cannot be \\r or \\n");
+ }
+ } else if (ARROW_PREDICT_FALSE(delimiter_string.find_first_of("\r\n") !=
+ std::string::npos)) {
+ return Status::Invalid("ParseOptions: delimiter_string cannot contain \\r
or \\n");
+ }
+ // The chunker handles quotes before delimiter matching at field boundaries,
so
+ // allowing the quote character at the start of a delimiter could make it
disagree
+ // with the parser.
Review Comment:
Why not be stricter and not allow the quote character at all?
If we ensure the delimiter doesn't contain any other structural character,
it may make further optimizations easier.
##########
cpp/src/arrow/csv/chunker_test.cc:
##########
@@ -193,6 +193,47 @@ TEST_P(BaseChunkerTest, QuotingNewline) {
}
}
+TEST_P(BaseChunkerTest, MultiDelimiter) {
+ if (!options_.newlines_in_values) {
+ return;
Review Comment:
Use `GTEST_SKIP` here?
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -341,16 +358,31 @@ class BlockParserImpl {
FieldStart:
// At the start of a field
- if (*data == options_.delimiter) {
- // Empty cells are very common in some files, shortcut them
- StartField(false /* quoted */);
- FinishField();
- ++data;
- ++num_cols;
- if (ARROW_PREDICT_FALSE(data == data_end)) {
- goto AbortLine;
+ {
+ size_t delimiter_len = 0;
+ if constexpr (SpecializedOptions::multi_delimiter) {
+ const auto delimiter = internal::GetDelimiter(options_);
Review Comment:
If we have `SpecializedOptions::multi_delimiter`, then we should always be
using `options_.delimiter_string`, right?
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -341,16 +358,31 @@ class BlockParserImpl {
FieldStart:
// At the start of a field
- if (*data == options_.delimiter) {
- // Empty cells are very common in some files, shortcut them
- StartField(false /* quoted */);
- FinishField();
- ++data;
- ++num_cols;
- if (ARROW_PREDICT_FALSE(data == data_end)) {
- goto AbortLine;
+ {
+ size_t delimiter_len = 0;
+ if constexpr (SpecializedOptions::multi_delimiter) {
+ const auto delimiter = internal::GetDelimiter(options_);
+ const auto delimiter_match = MatchDelimiter(delimiter, data, data_end,
is_final);
+ if (ARROW_PREDICT_FALSE(delimiter_match == DelimiterMatch::kPartial)) {
+ goto AbortLine;
+ }
+ if (ARROW_PREDICT_FALSE(delimiter_match == DelimiterMatch::kMatch)) {
+ delimiter_len = delimiter.size();
+ }
+ } else if (*data == options_.delimiter) {
+ delimiter_len = 1;
+ }
+ if (ARROW_PREDICT_FALSE(delimiter_len > 0)) {
Review Comment:
`ARROW_PREDICT_FALSE` should only mark exceptional cases (such as errors or
EOF), as the compiler might deoptimize those code paths. Here the comment says
`Empty cells are very common in some files`...
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -61,6 +64,58 @@ decltype(auto) DispatchBool(Fn&& fn, Rest... rest)
}
}
+inline std::string_view GetDelimiter(const ParseOptions& options) {
+ if (!options.delimiter_string.empty()) {
+ return options.delimiter_string;
+ }
+ return {&options.delimiter, 1};
+}
+
+inline char GetDelimiterFirstByte(const ParseOptions& options) {
+ if (!options.delimiter_string.empty()) {
+ return options.delimiter_string.front();
+ }
+ return options.delimiter;
+}
+
+class StreamingDelimiterMatcher {
+ public:
+ explicit StreamingDelimiterMatcher(std::string_view delimiter) :
delimiter_(delimiter) {
+ DCHECK(!delimiter_.empty());
+ }
+
+ bool Consume(char c) {
+ if (c == delimiter_[matched_]) {
+ matched_++;
+ if (matched_ == delimiter_.size()) {
+ matched_ = 0;
+ return true;
+ }
+ return false;
+ }
+
+ const std::string_view delimiter(delimiter_);
+ for (size_t length = matched_; length > 0; --length) {
+ if (c == delimiter[length - 1] &&
+ delimiter.substr(0, length - 1) ==
+ delimiter.substr(matched_ - (length - 1), length - 1)) {
Review Comment:
This is of course expensive and could easily be precomputed in a table,
since it only depends on the `delimiter_` pattern (a bit like in a Boyer-Moore
search I guess).
##########
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 { kNoMatch, kMatch, kPartial };
+
+DelimiterMatch MatchDelimiter(std::string_view delimiter, const char* data,
Review Comment:
Mark this `inline`?
##########
cpp/src/arrow/csv/reader_test.cc:
##########
@@ -620,6 +620,115 @@ TEST(ReaderTests, DefaultColumnTypeAllStringsNoHeader) {
ASSERT_TRUE(table->Equals(*expected_table));
}
+TEST(ParseOptions, DelimiterLineEndings) {
Review Comment:
Nit: can we cluster the `ParseOptions` tests separately from the
`ReaderTests` ones?
##########
cpp/src/arrow/csv/chunker.cc:
##########
@@ -262,6 +290,21 @@ class Lexer {
const ParseOptions& options_;
const BulkFilterType bulk_filter_;
State state_ = FIELD_START;
+
+ private:
+ struct Empty {};
+
+ static auto InitDelimiterMatcher(const ParseOptions& options) {
+ if constexpr (SpecializedOptions::multi_delimiter) {
+ return
internal::StreamingDelimiterMatcher(internal::GetDelimiter(options));
+ } else {
+ return Empty{};
+ }
+ }
+
+ [[no_unique_address]]
+ std::conditional_t<SpecializedOptions::multi_delimiter,
+ internal::StreamingDelimiterMatcher, Empty>
delimiter_matcher_;
Review Comment:
This is kludgy, can we just use `std::optional<StreamingDelimiterMatcher>`?
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -393,9 +426,21 @@ class BlockParserImpl {
}
goto InField;
}
- if (ARROW_PREDICT_FALSE(c == options_.delimiter)) {
+ if constexpr (SpecializedOptions::multi_delimiter) {
+ const auto delimiter = internal::GetDelimiter(options_);
+ const auto delimiter_match = MatchDelimiter(delimiter, data, data_end,
is_final);
+ if (ARROW_PREDICT_FALSE(delimiter_match == DelimiterMatch::kPartial)) {
+ goto AbortLine;
+ }
+ if (ARROW_PREDICT_FALSE(delimiter_match == DelimiterMatch::kMatch)) {
+ data += delimiter.size();
+ goto FieldEnd;
+ }
+ } else if (ARROW_PREDICT_FALSE(c == options_.delimiter)) {
+ ++data;
Review Comment:
This code looks very much like the one at `FieldStart`, is there a way to
factor it out?
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -268,67 +287,141 @@ TEST(BlockParser, Basics) {
}
}
-TEST(BlockParser, PadShortRows) {
+TEST(BlockParser, MultiDelimiter) {
auto options = ParseOptions::Defaults();
- options.pad_short_rows = true;
+ options.delimiter_string = "||";
- BlockParser parser(options, /*num_cols=*/3);
- AssertParseOk(parser, "1,2\n3,4,5\n");
- AssertColumnEq(parser, 0, {"1", "3"});
- AssertColumnEq(parser, 1, {"2", "4"});
- std::vector<std::string> values;
- std::vector<bool> missing;
- ASSERT_OK(parser.VisitColumn(
- 2, [&](const uint8_t* data, uint32_t size, bool, bool is_missing) ->
Status {
- values.emplace_back(reinterpret_cast<const char*>(data), size);
- missing.push_back(is_missing);
- return Status::OK();
- }));
- ASSERT_EQ(values, std::vector<std::string>({"", "5"}));
- ASSERT_EQ(missing, std::vector<bool>({true, false}));
-
- BlockParser last_row_parser(options, /*num_cols=*/3);
- AssertParseOk(last_row_parser, "1,2\n");
- std::vector<bool> last_row_missing;
- ASSERT_OK(last_row_parser.VisitLastRow(
- [&](const uint8_t*, uint32_t, bool, bool is_missing) -> Status {
- last_row_missing.push_back(is_missing);
- return Status::OK();
- }));
- ASSERT_EQ(last_row_missing, std::vector<bool>({false, false, true}));
+ {
Review Comment:
Can we add more single pipes in all those examples?
--
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]
