Skip to content

GH-51275: [C++][GLib] Allow the CSV Reader to accept multi-character delimiters #51286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HuaHuaY wants to merge 6 commits into
base: main
Choose a base branch
from
Open

GH-51275: [C++][GLib] Allow the CSV Reader to accept multi-character delimiters #51286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b94b6fe
allow the CSV Reader to accept multi-character delimiters
HuaHuaY Sep 10, 2026
bd6eb40
fix ci
HuaHuaY Sep 10, 2026
dbe45a9
address review comments
HuaHuaY Oct 9, 2026
a7912ea
address review comments
HuaHuaY Oct 10, 2026
19961af
fix ci
HuaHuaY Oct 10, 2026
612b963
address review comments
HuaHuaY Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions c_glib/arrow-glib/reader.cpp
View file
Open in desktop

@kou kou Oct 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This GLib part changes are correct but you can revert this in this PR. I can work on the GLib part including multi-character delimiters support as a follow-up task after this is merged.

@HuaHuaY HuaHuaY Oct 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert this part of the code tomorrow. However, this PR adds a std::string field in ParseOptions, which will break two CI tests because the reverted initialization code is missing in GLib.

@kou kou Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. If this part is required to keep our CI green, let's keep this part in this PR.

Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,18 @@ garrow_csv_read_options_dispose(GObject *object)
G_OBJECT_CLASS(garrow_csv_read_options_parent_class)->dispose(object);
}

static void
garrow_csv_read_options_finalize(GObject *object)
{
auto priv = GARROW_CSV_READ_OPTIONS_GET_PRIVATE(object);

priv->convert_options.~ConvertOptions();
priv->parse_options.~ParseOptions();
priv->read_options.~ReadOptions();

G_OBJECT_CLASS(garrow_csv_read_options_parent_class)->finalize(object);
}

static void
garrow_csv_read_options_set_property(GObject *object,
guint prop_id,
Expand Down Expand Up @@ -1052,6 +1064,9 @@ static void
garrow_csv_read_options_init(GArrowCSVReadOptions *object)
{
auto priv = GARROW_CSV_READ_OPTIONS_GET_PRIVATE(object);
new (&priv->read_options) arrow::csv::ReadOptions;
new (&priv->parse_options) arrow::csv::ParseOptions;
new (&priv->convert_options) arrow::csv::ConvertOptions;
priv->read_options = arrow::csv::ReadOptions::Defaults();
priv->parse_options = arrow::csv::ParseOptions::Defaults();
priv->convert_options = arrow::csv::ConvertOptions::Defaults();
Expand All @@ -1064,6 +1079,7 @@ garrow_csv_read_options_class_init(GArrowCSVReadOptionsClass *klass)

auto gobject_class = G_OBJECT_CLASS(klass);

gobject_class->finalize = garrow_csv_read_options_finalize;
gobject_class->dispose = garrow_csv_read_options_dispose;
gobject_class->set_property = garrow_csv_read_options_set_property;
gobject_class->get_property = garrow_csv_read_options_get_property;
Expand Down
101 changes: 69 additions & 32 deletions cpp/src/arrow/csv/chunker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,19 @@ class Lexer {
};

explicit Lexer(const ParseOptions& options)
: options_(options), bulk_filter_(options_) {
: options_(options),
bulk_filter_(options_),
delimiter_matcher_(InitDelimiterMatcher(options_)) {
DCHECK_EQ(SpecializedOptions::quoting, options_.quoting);
DCHECK_EQ(SpecializedOptions::escaping, options_.escaping);
}

void Reset() { state_ = FIELD_START; }
void Reset() {
state_ = FIELD_START;
if constexpr (SpecializedOptions::multi_delimiter) {
delimiter_matcher_.Reset();
}
}

// Decide whether it's worth using a bulk filter over the given data area
bool ShouldUseBulkFilter(const char* data, const char* data_end) {
Expand Down Expand Up @@ -126,13 +133,27 @@ class Lexer {

InField:
// Inside a non-quoted part of a field
if (UseBulkFilter) {
const char* bulk_end = RunBulkFilter(data, data_end);
if (ARROW_PREDICT_FALSE(bulk_end == nullptr)) {
state_ = IN_FIELD;
goto AbortLine;
if constexpr (UseBulkFilter) {
if constexpr (SpecializedOptions::multi_delimiter) {
if (!delimiter_matcher_.has_partial_match()) {
const char* bulk_end = RunBulkFilter(data, data_end);
if (ARROW_PREDICT_FALSE(bulk_end == nullptr)) {
state_ = IN_FIELD;
goto AbortLine;
}
data = bulk_end;
} else if (ARROW_PREDICT_FALSE(data == data_end)) {
state_ = IN_FIELD;
goto AbortLine;
}
} else {
const char* bulk_end = RunBulkFilter(data, data_end);
if (ARROW_PREDICT_FALSE(bulk_end == nullptr)) {
state_ = IN_FIELD;
goto AbortLine;
}
data = bulk_end;
}
data = bulk_end;
} else {
if (ARROW_PREDICT_FALSE(data == data_end)) {
state_ = IN_FIELD;
Expand All @@ -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();
}
Comment on lines +165 to +167

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the Reset call be done just at the start of FieldStart instead of in multiple places like this?

if (ARROW_PREDICT_FALSE(data == data_end)) {
state_ = AT_ESCAPE;
goto AbortLine;
Expand All @@ -157,9 +181,14 @@ class Lexer {
if (ARROW_PREDICT_FALSE(c == '\n')) {
goto LineEnd;
}
// treat delimiter as a normal token if quoting is disabled
if (ARROW_PREDICT_FALSE(SpecializedOptions::quoting && c == options_.delimiter)) {
goto FieldEnd;
if constexpr (SpecializedOptions::quoting) {
if constexpr (SpecializedOptions::multi_delimiter) {
if (ARROW_PREDICT_FALSE(delimiter_matcher_.Consume(c))) {
goto FieldStart;
}
} else if (ARROW_PREDICT_FALSE(c == options_.delimiter)) {
goto FieldStart;
}
}
goto InField;

Expand Down Expand Up @@ -223,12 +252,11 @@ class Lexer {
goto InField;
}

FieldEnd:
// At the end of a field
goto FieldStart;

LineEnd:
state_ = FIELD_START;
if constexpr (SpecializedOptions::multi_delimiter) {
delimiter_matcher_.Reset();
}
return data;

AbortLine:
Expand Down Expand Up @@ -262,6 +290,21 @@ class Lexer {
const ParseOptions& options_;
const BulkFilterType bulk_filter_;
State state_ = FIELD_START;

private:

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The private is a bit gratuitous here, no?

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_;
Comment on lines +305 to +307

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kludgy, can we just use std::optional<StreamingDelimiterMatcher>?

@HuaHuaY HuaHuaY Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because optional incurs memory overhead even when it’s not needed. But either is fine for me. I will modify it.

};

// A BoundaryFinder implementation that assumes CSV cells can contain raw newlines,
Expand Down Expand Up @@ -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();

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we actually check for delimiter_string.length() > 1?

delimiter = internal::DispatchBool(
[&]<bool Quoting, bool Escaping, bool MultiDelimiter>()
-> std::shared_ptr<BoundaryFinder> {
using SpecializedOptions =
internal::SpecializedOptions<Quoting, Escaping, MultiDelimiter>;
return std::make_shared<LexingBoundaryFinder<SpecializedOptions>>(options);
},
options.quoting, options.escaping, multi_delimiter);
}
return std::make_unique<Chunker>(std::move(delimiter));
}
Expand Down
41 changes: 41 additions & 0 deletions cpp/src/arrow/csv/chunker_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,47 @@ TEST_P(BaseChunkerTest, QuotingNewline) {
}
}

TEST_P(BaseChunkerTest, MultiDelimiter) {
if (!options_.newlines_in_values) {
return;

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use GTEST_SKIP here?

}
options_.delimiter_string = "||";
MakeChunker();

auto partial = std::make_shared<Buffer>("name|");
Comment thread
HuaHuaY marked this conversation as resolved.
auto block = std::make_shared<Buffer>("|message||\"hello\nworld\"||42\nnext||row\n");

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add more single pipes to exercise the delimiter search logic?

std::shared_ptr<Buffer> completion;
std::shared_ptr<Buffer> rest;
ASSERT_OK(chunker_->ProcessWithPartial(partial, block, &completion, &rest));
ASSERT_EQ(completion->ToString(), "|message||\"hello\nworld\"||42\n");
ASSERT_EQ(rest->ToString(), "next||row\n");

partial = std::make_shared<Buffer>("name|");
block = std::make_shared<Buffer>("|message");
ASSERT_RAISES(Invalid,
chunker_->ProcessWithPartial(partial, block, &completion, &rest));

// Long ordinary fields make the bulk filter profitable.
for (size_t padding : {0, 256}) {
const std::string first_field = std::string(padding, 'a') + "a|b";
const std::string row = first_field + "||\"c\n d\"||e\n";
const std::string next_row = std::string(padding, 'n') + "next||row\n";
MakeChunker();
const auto lengths = {row.size(), next_row.size()};
AssertChunking(*chunker_, row + next_row, lengths);

for (size_t split :
{first_field.find('|') + 1, first_field.size() + 1, first_field.size() + 2}) {
ARROW_SCOPED_TRACE("padding = ", padding, ", split = ", split);
partial = Buffer::FromString(row.substr(0, split));
block = Buffer::FromString(row.substr(split) + next_row);
ASSERT_OK(chunker_->ProcessWithPartial(partial, block, &completion, &rest));
ASSERT_EQ(completion->ToString(), row.substr(split));
ASSERT_EQ(rest->ToString(), next_row);
}
}
}

TEST_P(BaseChunkerTest, QuotingUnbalanced) {
// Quote introduces a quoted field that doesn't end
auto csv = MakeCSVData({"a,b\n", "1,\",3,,5\n", "c,d\n"});
Expand Down
63 changes: 59 additions & 4 deletions cpp/src/arrow/csv/lexing_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,25 @@

#include <cstdint>
#include <cstring>
#include <string>
#include <string_view>
#include <type_traits>
#include <utility>

#include "arrow/csv/options.h"
#include "arrow/util/logging_internal.h"
#include "arrow/util/simd.h"

namespace arrow {
namespace csv {
namespace internal {

template <bool Quoting, bool Escaping>
template <bool Quoting, bool Escaping, bool MultiDelimiter>
class SpecializedOptions {
public:
static constexpr bool quoting = Quoting;
static constexpr bool escaping = Escaping;
static constexpr bool multi_delimiter = MultiDelimiter;
};

/// Convert runtime boolean options into template arguments for a callable.
Expand All @@ -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)) {
Comment on lines +100 to +101

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

matched_ = length;
return false;
}
}
matched_ = 0;
return false;
}

void Reset() { matched_ = 0; }

bool has_partial_match() const { return matched_ != 0; }

private:
const std::string delimiter_;
size_t matched_ = 0;
};

//
// Bulk filters for packed character matching.
// These filters allow checking multiple CSV bytes at once for specific
Expand All @@ -87,7 +142,7 @@ class BaseBloomFilter {
auto add_char = [&](char c) { filter |= CharFilter(c); };
add_char('\n');
add_char('\r');
add_char(options.delimiter);
add_char(GetDelimiterFirstByte(options));
if (options.escaping) {
add_char(options.escape_char);
}
Expand Down Expand Up @@ -188,7 +243,7 @@ class SSE42Filter {
// Make a SIMD word of the characters we want to match
const char cr = '\r';
const char lf = '\n';
const char delim = options.delimiter;
const char delim = GetDelimiterFirstByte(options);
const char quote = SpecializedOptions::quoting ? options.quote_char : cr;
const char escape = SpecializedOptions::escaping ? options.escape_char : cr;

Expand All @@ -213,7 +268,7 @@ class NeonFilter {
using WordType = uint8x8_t;

explicit NeonFilter(const ParseOptions& options)
: delim_(vdup_n_u8(options.delimiter)),
: delim_(vdup_n_u8(GetDelimiterFirstByte(options))),
quote_(vdup_n_u8(SpecializedOptions::quoting ? options.quote_char : '\n')),
escape_(vdup_n_u8(SpecializedOptions::escaping ? options.escape_char : '\n')) {}

Expand Down
35 changes: 33 additions & 2 deletions cpp/src/arrow/csv/options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@wgtmac wgtmac Sep 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
  }

@HuaHuaY HuaHuaY Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, by doing this, we lose the specific reason for the error.

// character in a delimiter could make it disagree with the parser.
if (escaping) {
Comment thread
HuaHuaY marked this conversation as resolved.
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.
Comment on lines +50 to +52

@pitrou pitrou Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const char delimiter_first_byte =
delimiter_string.empty() ? delimiter : delimiter_string.front();
if (ARROW_PREDICT_FALSE(quoting && delimiter_first_byte == quote_char)) {
return Status::Invalid(
"ParseOptions: delimiter cannot start with the quote character when quoting is "
"enabled");
}
if (ARROW_PREDICT_FALSE(quoting && (quote_char == '\n' || quote_char == '\r'))) {
return Status::Invalid("ParseOptions: quote_char cannot be \\r or \\n");
Expand Down
Loading
Loading