westonpace commented on a change in pull request #11857:
URL: https://github.com/apache/arrow/pull/11857#discussion_r764588115
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -80,15 +80,17 @@ int64_t CountQuotes(util::string_view s) {
// Matching quote pair character length.
constexpr int64_t kQuoteCount = 2;
constexpr int64_t kQuoteDelimiterCount = kQuoteCount + /*end_char*/ 1;
+static const std::string& str_comma = ",";
// Interface for generating CSV data per column.
// The intended usage is to iteratively call UpdateRowLengths for a column and
// then PopulateColumns. PopulateColumns must be called in the reverse order
of the
// populators (it populates data backwards).
class ColumnPopulator {
public:
- ColumnPopulator(MemoryPool* pool, char end_char, std::shared_ptr<Buffer>
null_string)
- : end_char_(end_char), null_string_(std::move(null_string)), pool_(pool)
{}
+ ColumnPopulator(MemoryPool* pool, const std::string& end_chars,
+ std::shared_ptr<Buffer> null_string)
+ : end_chars_(end_chars), null_string_(std::move(null_string)),
pool_(pool) {}
Review comment:
I think I may have muddied the waters with my earlier comments. There
are generally two approaches we can take:
```
class ColumnPopulator {
ColumnPopulator(const std::string& end_chars) : end_chars_(end_chars) {}
protected:
const std::string& end_chars_;
}
```
Or...
```
class ColumnPopulator {
ColumnPopulator(std::string end_chars) : end_chars_(std::move(end_chars))
{}
protected:
const std::string end_chars_;
}
```
Your implementation is still kind of in between the two approaches. Also,
when I originally gave feedback I thought it would be ok to use either
approach. Now that I am looking more closely I can see that the first approach
will not work (I can explain more on this if you want). So we should aim for
the second approach:
```suggestion
ColumnPopulator(MemoryPool* pool, std::string end_chars,
std::shared_ptr<Buffer> null_string)
: end_chars_(std::move(end_chars)),
null_string_(std::move(null_string)), pool_(pool) {}
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -146,10 +148,10 @@ char* EscapeReverse(arrow::util::string_view s, char*
out_end) {
// compliance with RFC4180 section 2.5.
class UnquotedColumnPopulator : public ColumnPopulator {
public:
- explicit UnquotedColumnPopulator(MemoryPool* memory_pool, char end_char,
+ explicit UnquotedColumnPopulator(MemoryPool* memory_pool, const std::string&
end_chars,
std::shared_ptr<Buffer> null_string_,
bool reject_values_with_quotes)
- : ColumnPopulator(memory_pool, end_char, std::move(null_string_)),
+ : ColumnPopulator(memory_pool, end_chars, std::move(null_string_)),
Review comment:
```suggestion
: ColumnPopulator(memory_pool, std::move(end_chars),
std::move(null_string_)),
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -146,10 +148,10 @@ char* EscapeReverse(arrow::util::string_view s, char*
out_end) {
// compliance with RFC4180 section 2.5.
class UnquotedColumnPopulator : public ColumnPopulator {
public:
- explicit UnquotedColumnPopulator(MemoryPool* memory_pool, char end_char,
+ explicit UnquotedColumnPopulator(MemoryPool* memory_pool, const std::string&
end_chars,
std::shared_ptr<Buffer> null_string_,
bool reject_values_with_quotes)
Review comment:
```suggestion
explicit UnquotedColumnPopulator(MemoryPool* memory_pool, std::string
end_chars,
std::shared_ptr<Buffer> null_string_,
bool reject_values_with_quotes)
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -350,27 +355,27 @@ struct PopulatorFactory {
// is None.
case QuotingStyle::None:
case QuotingStyle::Needed:
- populator = new UnquotedColumnPopulator(pool, end_char, null_string,
+ populator = new UnquotedColumnPopulator(pool, end_chars, null_string,
/*reject_values_with_quotes=*/false);
break;
case QuotingStyle::AllValid:
- populator = new QuotedColumnPopulator(pool, end_char, null_string);
+ populator = new QuotedColumnPopulator(pool, end_chars, null_string);
break;
}
return Status::OK();
}
- char end_char;
+ std::string end_chars;
std::shared_ptr<Buffer> null_string;
const QuotingStyle quoting_style;
MemoryPool* pool;
ColumnPopulator* populator;
};
Result<std::unique_ptr<ColumnPopulator>> MakePopulator(
- const Field& field, char end_char, std::shared_ptr<Buffer> null_string,
+ const Field& field, const std::string& end_chars, std::shared_ptr<Buffer>
null_string,
Review comment:
```suggestion
const Field& field, std::string end_chars, std::shared_ptr<Buffer>
null_string,
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -80,15 +80,17 @@ int64_t CountQuotes(util::string_view s) {
// Matching quote pair character length.
constexpr int64_t kQuoteCount = 2;
constexpr int64_t kQuoteDelimiterCount = kQuoteCount + /*end_char*/ 1;
+static const std::string& str_comma = ",";
Review comment:
```suggestion
static const std::string kDefaultSeparator = ",";
```
The naming convention for constants is kVariableName.
Arrow follows the Google C++ style guide which is a good resource for these
kinds of tidbits:
https://google.github.io/styleguide/cppguide.html#Constant_Names
Some code will also use ALL_CAPS_STYLE but that should be avoided if you can
(sometimes we cannot due to interoperability with other libs).
Also, for a constant string we don't need the `&`
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -227,9 +231,9 @@ class UnquotedColumnPopulator : public ColumnPopulator {
// a quote character (") and escaping is done my adding another quote.
class QuotedColumnPopulator : public ColumnPopulator {
public:
- QuotedColumnPopulator(MemoryPool* pool, char end_char,
+ QuotedColumnPopulator(MemoryPool* pool, std::string& end_chars,
std::shared_ptr<Buffer> null_string)
- : ColumnPopulator(pool, end_char, std::move(null_string)) {}
+ : ColumnPopulator(pool, end_chars, std::move(null_string)) {}
Review comment:
```suggestion
: ColumnPopulator(pool, std::move(end_chars), std::move(null_string))
{}
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -227,9 +231,9 @@ class UnquotedColumnPopulator : public ColumnPopulator {
// a quote character (") and escaping is done my adding another quote.
class QuotedColumnPopulator : public ColumnPopulator {
public:
- QuotedColumnPopulator(MemoryPool* pool, char end_char,
+ QuotedColumnPopulator(MemoryPool* pool, std::string& end_chars,
Review comment:
```suggestion
QuotedColumnPopulator(MemoryPool* pool, std::string end_chars,
```
##########
File path: cpp/src/arrow/csv/writer.cc
##########
@@ -350,27 +355,27 @@ struct PopulatorFactory {
// is None.
case QuotingStyle::None:
case QuotingStyle::Needed:
- populator = new UnquotedColumnPopulator(pool, end_char, null_string,
+ populator = new UnquotedColumnPopulator(pool, end_chars, null_string,
/*reject_values_with_quotes=*/false);
break;
case QuotingStyle::AllValid:
- populator = new QuotedColumnPopulator(pool, end_char, null_string);
+ populator = new QuotedColumnPopulator(pool, end_chars, null_string);
break;
}
return Status::OK();
}
- char end_char;
+ std::string end_chars;
std::shared_ptr<Buffer> null_string;
const QuotingStyle quoting_style;
MemoryPool* pool;
ColumnPopulator* populator;
};
Result<std::unique_ptr<ColumnPopulator>> MakePopulator(
- const Field& field, char end_char, std::shared_ptr<Buffer> null_string,
+ const Field& field, const std::string& end_chars, std::shared_ptr<Buffer>
null_string,
QuotingStyle quoting_style, MemoryPool* pool) {
- PopulatorFactory factory{end_char, std::move(null_string), quoting_style,
pool,
+ PopulatorFactory factory{end_chars, std::move(null_string), quoting_style,
pool,
Review comment:
```suggestion
PopulatorFactory factory{std::move(end_chars), std::move(null_string),
quoting_style, pool,
```
--
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]
