You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kevingurney (via GitHub)" <gi...@apache.org> on 2023/10/02 20:44:28 UTC
[PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
kevingurney opened a new pull request, #37981:
URL: https://github.com/apache/arrow/pull/37981
### Rationale for this change
In order to make the [`arrow::PrettyPrint`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L101) functionality for `arrow::Array` more flexible, it would be useful to be to specify a custom `Array` element delimiter other than `","`.
For example, the MATLAB interface wraps the Arrow C++ libraries and being able to specify a custom `Array` element delimiter, would make it possible to make the display of MATLAB `arrow.array.Array` objects more MATLAB-like.
For the MATLAB interface, we would like to enable display that looks something like the following (note the ` | ` between individual `Array` elements:
```matlab
% Make a MATLAB array.
>> A = 1:5
A =
1 2 3 4 5
% Make an Arrow array from the MATLAB array.
>> B = arrow.array(A)
B =
[ 1 | 2 | 3 | 4 | 5 ]
```
In order to support custom `Array` element delimiters, this pull request adds a new option `array_element_delimiter` with type `std::string` to the [`arrow::PrettyPrintOptions`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L35) struct.
In a future pull request, we would like to enable more `PrettyPrintOptions` like custom closing and opening delimiters for `arrow::Array` (e.g. "<" rather than "[" and ">" rather than "]").
### What changes are included in this PR?
1. Added a new option `array_element_delimiter` with type `std::string` to the [`arrow::PrettyPrintOptions`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L35) struct. This option allows the user to specify any string value as the delimiter to distinguish between individual elements of an `arrow::Array` when being printed using [`arrow::PrettyPrint`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L101).
### Are these changes tested?
Yes.
1. Added new tests `PrimitiveTypeCustomArrayElementDelimiter` and `ChunkedArrayPrimitiveTypeCustomArrayDelimiter` to `pretty_print_test.cc`.
2. All existing `PrettyPrint`-related C++ tests pass.
### Are there any user-facing changes?
Yes.
1. User's can now specify a custom `Array` element delimiter when printing `arrow::Array`s using [`arrow::PrettyPrint`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L101) by specifying a value for the `array_element_delimiter` property of [`arrow::PrettyPrintOptions`](https://github.com/apache/arrow/blob/7667b81bffcb5b361fab6d61c42ce396d98cc6e1/cpp/src/arrow/pretty_print.h#L35).
### Future Directions
1. To keep this pull request small and focused, I intentionally chose not to include changes related to specifying custom opening and closing `Array` delimiters (e.g. use `<` and `>` instead of `[` and `]`). I've captured the idea of supporting custom opening and closing `Array` delimiters in #37979. I will follow up with a future PR to address this.
### Notes
1. This pull request was motivated by our desire to improve the display of Arrow related classes in the MATLAB interface, but it is hopefully a generic enough change that it may benefit other use cases too.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343295807
##########
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, good question.
I considered using a shorter name like just `element_delimiter`, but I included `array_` because I thought that if we end up wanting to expose more options like the ability to specify a unique delimiter for `ChunkedArray` elements, that it might become ambiguous. In other words, it might be helpful to have more `array_element_delimiter` and `chunked_array_element_delimiter`.
Do you think we should still remove `array_` given this consideration?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343270462
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
Ah, we need to add one more option for array delimiter for the `,` to solve GH-30951.
(Anyway, GH-30951 is out of scope of this issue/PR.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1346114936
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
**Update**: I realized I only updated one of the tests before to not modify the value of `skip_new_lines` or `window`. I just made a change to update the other test. Sorry for missing this before.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343299377
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
Yes, that sounds right.
See my comment: https://github.com/apache/arrow/pull/37981#discussion_r1343295807.
To fully address https://github.com/apache/arrow/issues/30951, I think we could have two properties: (1) `array_element_delimiter` and (2) `chunked_array_element_delimiter`. The `chunked_array_element_delimiter` would allow client code to control which delimiter is used to separate individual chunks in the `ChunkedArray` display.
Let me know if you like this approach. If so, I can follow up with another PR to add `chunked_array_element_delimiter`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343340397
##########
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:
For `ChunkedArray`, I think that `array_delimiter` is better.
Because `ChunkedArray`'s element is an `Array`. `array_` is more concrete than `element_` for `ChunkedArray`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
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 open an Array.
std::string opening_delimiter = "[";
// Delimiter used to close an Array.
std::string closing_delimiter = "]";
// Delimiter used to separate individual scalar elements in an Array.
std::string element_delimiter = ", ";
}
struct ChunkedArrayPrettyPrintOptions {
// Delimiter used to open a ChunkedArray.
std::string opening_delimiter = "[";
// Delimiter used to close a ChunkedArray.
std::string closing_delimiter = "]";
// Delimiter used to separate individual chunks (i.e. Arrays) in a ChunkedArray.
std::string element_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.opening_delimiter = "{";
options.array_options.closing_delimiter = "}";
options.array_options.element_delimiter = " | ";
options.chunked_array_options.element_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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1345016394
##########
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:
Interesting.
Then how about the following? I want to try simplifying your idea.
```cpp
struct PrettyPrintDelimiters {
std::string open = "[";
std::string close = "]";
std::string element = ",";
};
struct PrettyPrintOptions {
// ...
PrettyPrintDelimiters array_delimiters = PrettyPrintDelimiters::Defaults();
PrettyPrintDelimiters chunked_array_delimiters = PrettyPrintDelimiters::Defaults();
// ...
};
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1346118767
##########
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:
Ok, I've implemented the `PrettyPrintDelimiters` approach as suggested.
I intentionally chose not to add `open` or `close` yet because my plan is to follow up with a separate PR to address this since that functionality is also captured by the separate issue https://github.com/apache/arrow/issues/37979. As soon as this is merged in, I will work on adding `open` and `close`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1751844099
I think that it's a false positive...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343296664
##########
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:
Correct. From my understanding this should be relatively straightforward if we still want to change the default display.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1750855272
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit f5e40dc45ba3465dfaf1c32af020b63a294b6982.
There were 6 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-10-06 08:03:49Z](https://conbench.ursa.dev/compare/runs/141651da421049f3b1669d7a3f5a4d88...69103244469d4b01bbe493f39194a3fb/)
- [`dataset-selectivity` (Python) with dataset=nyctaxi_multi_ipc_s3, selectivity=10%](https://conbench.ursa.dev/compare/benchmarks/0651f9afb7a378b38000b5076b245056...0651fc432aa176c680004ca40db9d793)
- [`dataset-selectivity` (Python) with dataset=nyctaxi_multi_parquet_s3, selectivity=100%](https://conbench.ursa.dev/compare/benchmarks/0651f9ae9815726080000dd74f4e0a39...0651fc4168cd7b338000242f45652768)
- and 4 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/17470237222) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1344495447
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
Marking this as resolved since the main discussion around naming/design is happening in https://github.com/apache/arrow/pull/37981#discussion_r1343266917.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1344498869
##########
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:
Marking this as resolved since we can address https://github.com/apache/arrow/issues/30951 in a follow up PR by changing the default value of `array_element_delimiter` (or whatever name we decide on using).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney merged PR #37981:
URL: https://github.com/apache/arrow/pull/37981
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1751400679
This PR appears to have triggered a regression, but I am not clear why that would be. I wouldn't have expected the overhead of accessing a nested property on `PrettyPrintOptions` to be so high.
Do these benchmarks exercise the `PrettyPrint` code heavily? Perhaps, this run is an outlier?
I am new to interpreting Conbench results, so please let me know if there is more I can do to understand what is happening here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343297417
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
Hmm, that's a good point.
I included these because I wanted to emulate the behavior of how we would like the MATLAB display to work (i.e. horizontal display with no new lines and a small window), but I am happy to remove these to keep the test more focused.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1743999418
@kou - thanks for the helpful review!
Sorry for the delay in fixing the C++ linting errors. I am having some trouble getting `ninja format` to work as expected.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1344494522
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1347675325
##########
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:
Marking this as resolved.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1744056276
@kou - thank you for offering! That would be very helpful if it wouldn't be too much trouble.
Unfortunately I think it will take some time for me to sort out the machine configuration issues I am having.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1744029730
Sorry, I am still having machine configuration issues. I will try to get this resolved tomorrow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1751403686
One of the benchmarks that may have regressed is [`dataframe-to-table`](https://github.com/voltrondata-labs/benchmarks/blob/main/benchmarks/dataframe_to_table_benchmark.py). However, I am confused by this because it seems that this benchmark essentially reads a Parquet file and then immediately converts to a Pandas DataFrame, so I am not sure why this would even be using the `PrettyPrint` code path.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1744079228
Sure! But I couldn't push to the branch... Could you apply the following patch?
```diff
diff --git a/cpp/src/arrow/pretty_print_test.cc b/cpp/src/arrow/pretty_print_test.cc
index 6c57505a3..cdced96e7 100644
--- a/cpp/src/arrow/pretty_print_test.cc
+++ b/cpp/src/arrow/pretty_print_test.cc
@@ -212,38 +212,19 @@ TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
// Short array without ellipsis
{
- std::vector<bool> is_valid = {true, true, false, true, false};
- std::vector<int32_t> values = {1, 2, 3, 4, 5};
- const char* expected = "[1 | 2 | null | 4 | null]";
- CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
+ std::vector<bool> is_valid = {true, true, false, true, false};
+ std::vector<int32_t> values = {1, 2, 3, 4, 5};
+ const char* expected = "[1 | 2 | null | 4 | null]";
+ CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
// Longer array with ellipsis
{
- std::vector<bool> is_valid = {
- true,
- false,
- true,
- true,
- false,
- true,
- false,
- true,
- true
- };
- std::vector<int32_t> values = {
- 1,
- 2,
- 3,
- 4,
- 5,
- 6,
- 7,
- 8,
- 9
- };
- const char* expected = "[1 | null | 3 | ... | null | 8 | 9]";
- CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
+ std::vector<bool> is_valid = {true, false, true, true, false,
+ true, false, true, true};
+ std::vector<int32_t> values = {1, 2, 3, 4, 5, 6, 7, 8, 9};
+ const char* expected = "[1 | null | 3 | ... | null | 8 | 9]";
+ CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
}
@@ -1081,20 +1062,20 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
// ChunkedArray with 1 chunk
{
- const ChunkedArray chunked_array(chunk);
+ const ChunkedArray chunked_array(chunk);
- static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
- CheckStream(chunked_array, options, expected);
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
}
// ChunkedArray with 2 chunks
{
- const ChunkedArray chunked_array({chunk, chunk});
+ const ChunkedArray chunked_array({chunk, chunk});
- static const char* expected =
- R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
- CheckStream(chunked_array, options, expected);
+ CheckStream(chunked_array, options, expected);
}
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1744037019
Do you want me to run `ninja format` and push the fix?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343267311
##########
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:
We can change the default value to `", "` in a separated PR to solve GH-30951, right?
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
How about removing them because we want to focus on `array_element_delimiter` in this test?
##########
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:
How about removing `array_`?
It seems that `array_` is redundant.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343295807
##########
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, good question.
I considered using a shorter name like just `element_delimiter`, but I included `array_` because I thought that if we end up wanting to expose more options like the ability to specify a unique delimiter for `ChunkedArray` elements, that it might become ambiguous. In other words, it might be helpful to have both `array_element_delimiter` and `chunked_array_element_delimiter`.
Do you think we should still remove `array_` given this consideration?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1753125937
OK, I will treat it as a false positive for the time being, in that case.
In the future, if we wanted, we could follow up with another PR to store the custom delimiter values in local variables inside of `WriteValues`. I'm not sure, but, there is a chance that might be slightly more efficient than repeatedly indexing into a nested struct.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343345845
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
I understand your thought.
I think that we can remove them because we will be able to detect whether mixing these options is broken or not in MATLAB tests.
Anyway, we can keep these options here if you want. :-)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1744008113
@github-actions autotune
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1344134921
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -200,6 +200,53 @@ TEST_F(TestPrettyPrint, PrimitiveTypeNoNewlines) {
CheckPrimitive<Int32Type, int32_t>(options, is_valid, values, expected, false);
}
+TEST_F(TestPrettyPrint, PrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
Review Comment:
Great! I agree. Since it is simple enough for me to modify these tests to not include the other options and since I think it is a good idea to keep these tests as focused as possible, I will go ahead and remove the other options as you originally suggested.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1749254491
I have updated the description. Thanks for the reminder!
At this point, I think this is ready to be merged, so I will go ahead and merge it.
Thanks very much for your help @kou!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1749261940
+1
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1745456833
Thanks for providing the patch @kou! I've applied it and made some edits to the tests based on your suggestions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37981:
URL: https://github.com/apache/arrow/pull/37981#issuecomment-1743768671
I will address the C++ linting errors shortly, my apologies for the delay.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343299377
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
Yes, I believe that is right.
See my comment: https://github.com/apache/arrow/pull/37981#discussion_r1343295807.
To fully address https://github.com/apache/arrow/issues/30951, I think we could have two properties: (1) `array_element_delimiter` and (2) `chunked_array_element_delimiter`.
Let me know if you like this approach and I can follow up with another PR to add `chunked_array_element_delimiter`.
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
Yes, that sounds right.
See my comment: https://github.com/apache/arrow/pull/37981#discussion_r1343295807.
To fully address https://github.com/apache/arrow/issues/30951, I think we could have two properties: (1) `array_element_delimiter` and (2) `chunked_array_element_delimiter`.
Let me know if you like this approach and I can follow up with another PR to add `chunked_array_element_delimiter`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1343348097
##########
cpp/src/arrow/pretty_print_test.cc:
##########
@@ -1020,6 +1067,37 @@ TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveType) {
CheckStream(chunked_array_2, {0}, expected_2);
}
+TEST_F(TestPrettyPrint, ChunkedArrayPrimitiveTypeCustomArrayElementDelimiter) {
+ PrettyPrintOptions options{};
+ // Display array contents on one line.
+ options.skip_new_lines = true;
+ // Display maximum of 3 elements at the beginning and at the end of the array
+ options.window = 3;
+ // Use a custom array element delimiter of " | ",
+ // rather than the default delimiter (i.e. ",").
+ options.array_element_delimiter = " | ";
+
+ const auto chunk = ArrayFromJSON(int32(), "[1, 2, null, 4, null]");
+
+ // ChunkedArray with 1 chunk
+ {
+ const ChunkedArray chunked_array(chunk);
+
+ static const char* expected = R"expected([[1 | 2 | null | 4 | null]])expected";
+ CheckStream(chunked_array, options, expected);
+ }
+
+ // ChunkedArray with 2 chunks
+ {
+ const ChunkedArray chunked_array({chunk, chunk});
+
+ static const char* expected =
+ R"expected([[1 | 2 | null | 4 | null],[1 | 2 | null | 4 | null]])expected";
Review Comment:
I think that `element_delimiter` and `array_delimiter` are better for naming.
Let's discuss it in the https://github.com/apache/arrow/pull/37981#discussion_r1343266917 thread.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37978: [C++] Add support for specifying custom Array element delimiter to `arrow::PrettyPrintOptions` [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37981:
URL: https://github.com/apache/arrow/pull/37981#discussion_r1345909439
##########
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:
Thanks for working on this with me!
I really like this approach!
The simple names `open`, `close`, and `element` are definitely an improvement over the ones I suggested. Also, this approach is nice because it only requires adding one additional `struct` type, rather than two.
I will proceed with implementing this approach.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org