kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1344158510
##########
cpp/src/arrow/pretty_print.h:
##########
@@ -77,6 +77,9 @@ struct PrettyPrintOptions {
/// If true, display schema metadata when pretty-printing a Schema
bool show_schema_metadata = true;
+
+ /// Delimiter for separating individual elements of an Array (e.g. ",")
+ std::string array_element_delimiter = ",";
Review Comment:
Hmm, that's an interesting suggestion. My reasoning for suggesting the names
`array_element_delimiter` and `chunked_array_element_delimiter` was that an
"element" could be considered a generic term for whatever a "container type"
(i.e. `Array`, `ChunkedArray`, etc.) uses as its "internal atomic unit". For
`Array` - the "internal atomic unit" would be a single scalar element. For
`ChunkedArray` - the "internal atomic unit" would be an `Array` (as you rightly
pointed out).
The only potential concern I see with using the name `array_delimiter` is
that it might be slightly ambiguous - e.g. "Is it the delimiter that opens /
closes an *Array* (i.e. the "[" and "]") or is it the *internal* element-wise
delimiter of an *Array*?" I was thinking that it might be helpful to explicitly
point out whether a property is `Array`-related and `ChunkedArray`-related
through the name.
However, thinking through this more gave me another idea, which might be a
bit more scalable.
What if we instead had "nested" options that control the behavior of
specific types?
For example:
```cpp
struct ArrayPrettyPrintOptions {
// Delimiter used to separate individual scalar elements in an Array.
std::string delimiter = ", ";
}
struct ChunkedArrayPrettyPrintOptions {
// Delimiter used to separate individual chunks (i.e. Arrays) in a
ChunkedArray.
std::string delimiter = ", ";
}
struct PrettyPrintOptions {
.
.
.
ArrayPrettyPrintOptions array_options =
ArrayPrettyPrintOptions::Defaults();
ChunkedArrayPrettyPrintOptions chunked_array_options =
ChunkedArrayPrettyPrintOptions::Defaults();
/// Number of spaces to shift entire formatted object to the right
int indent = 0;
/// Size of internal indents
int indent_size = 2;
.
.
.
};
```
Then in client code - you could set these options like:
```cpp
PrettyPrintOptions options = PrettyPrintOptions::Defaults();
options.array_options.delimiter = " | ";
options.chunked_array_options.delimiter = ", ";
```
Following this pattern might be more extensible to other types in the
future, as well (e.g. `Table`, `Schema`, etc.).
---
All that being said - I am OK with using `array_delimiter` and
`element_delimiter` as the names if you feel like these are still preferred. My
primary goal in choosing these names is just to make sure they are as clear and
unambiguous as possible.
--
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]
