pitrou commented on a change in pull request #12091:
URL: https://github.com/apache/arrow/pull/12091#discussion_r786581854
##########
File path: cpp/src/arrow/pretty_print.h
##########
@@ -36,12 +36,13 @@ struct PrettyPrintOptions {
PrettyPrintOptions() = default;
PrettyPrintOptions(int indent_arg, // NOLINT runtime/explicit
- int window_arg = 10, int indent_size_arg = 2,
- std::string null_rep_arg = "null", bool
skip_new_lines_arg = false,
- bool truncate_metadata_arg = true)
+ int window_arg = 10, int container_window_arg = 2,
+ int indent_size_arg = 2, std::string null_rep_arg =
"null",
+ bool skip_new_lines_arg = false, bool
truncate_metadata_arg = true)
Review comment:
Unfortunately we cannot add an argument in the middle without breaking
the API. We can only add arguments at the end (which may explain why the
signature is as whacky).
##########
File path: cpp/src/arrow/pretty_print_test.cc
##########
@@ -715,17 +715,35 @@ TEST_F(TestPrettyPrint, ListType) {
3
]
])expected";
+ static const char* ex_4 = R"expected([
+ [
+ null
+ ],
+ [],
+ ...
+ [
+ 4,
+ 6,
+ 7
+ ],
+ [
+ 2,
+ 3
+ ]
+])expected";
auto array = ArrayFromJSON(list_type, "[[null], [], null, [4, 6, 7], [2,
3]]");
- CheckArray(*array, {0, 10}, ex);
- CheckArray(*array, {2, 10}, ex_2);
- CheckStream(*array, {0, 1}, ex_3);
+ CheckArray(*array, {0, 10, 5}, ex, false);
Review comment:
It would be nice to make the constructor arguments more explicit here
and in other places you're changin, e.g.:
```suggestion
CheckArray(*array, {/*indent=*/0, /*window=*/10, /*container_window=*/5},
ex, false);
```
##########
File path: cpp/src/arrow/pretty_print.cc
##########
@@ -418,13 +424,16 @@ Status PrettyPrint(const ChunkedArray& chunked_arr, const
PrettyPrintOptions& op
std::ostream* sink) {
int num_chunks = chunked_arr.num_chunks();
int indent = options.indent;
- int window = options.window;
+ int window = options.container_window;
+ // Struct fields are always on new line
Review comment:
Is there a test for that? I didn't see anything in this PR's changes,
but perhaps I missed something?
##########
File path: cpp/src/arrow/pretty_print.h
##########
@@ -36,12 +36,13 @@ struct PrettyPrintOptions {
PrettyPrintOptions() = default;
PrettyPrintOptions(int indent_arg, // NOLINT runtime/explicit
- int window_arg = 10, int indent_size_arg = 2,
- std::string null_rep_arg = "null", bool
skip_new_lines_arg = false,
- bool truncate_metadata_arg = true)
+ int window_arg = 10, int container_window_arg = 2,
+ int indent_size_arg = 2, std::string null_rep_arg =
"null",
+ bool skip_new_lines_arg = false, bool
truncate_metadata_arg = true)
Review comment:
By the way, can you remove the "arg_" at the end of all argument names?
It's pointless.
##########
File path: python/pyarrow/table.pxi
##########
@@ -74,9 +74,13 @@ cdef class ChunkedArray(_PandasConvertible):
How much to indent right the content of the array,
by default ``0``.
window : int
- How many items to preview at the begin and end
- of the array when the arrays is bigger than the window.
+ How many items to preview within each chunk at the begin and end
+ of the chunk when the chunk is bigger than the window.
The other elements will be ellipsed.
+ container_window : int
+ How many chunks to preview at the begin and end
+ of the array when the arrays is bigger than the window.
+ The other elements will be ellipsed. Window also applies to list
columns.
Review comment:
```suggestion
The other elements will be ellipsed.
This setting also applies to list columns.
```
##########
File path: python/pyarrow/array.pxi
##########
@@ -1019,9 +1019,13 @@ cdef class Array(_PandasConvertible):
How much to indent right the entire content of the array,
by default ``0``.
window : int
- How many items to preview at the begin and end
- of the array when the arrays is bigger than the window.
+ How many primitive items to preview at the begin and end
Review comment:
"items" vs "elements" below?
##########
File path: python/pyarrow/array.pxi
##########
@@ -1019,9 +1019,13 @@ cdef class Array(_PandasConvertible):
How much to indent right the entire content of the array,
by default ``0``.
window : int
- How many items to preview at the begin and end
- of the array when the arrays is bigger than the window.
+ How many primitive items to preview at the begin and end
+ of the array when the array is bigger than the window.
The other elements will be ellipsed.
+ container_window : int
+ How many container elements (such as a list in a list array)
+ at the begin and end of the array when the array is bigger
Review comment:
Missing "to preview" here?
##########
File path: cpp/src/arrow/pretty_print_test.cc
##########
@@ -715,17 +715,35 @@ TEST_F(TestPrettyPrint, ListType) {
3
]
])expected";
+ static const char* ex_4 = R"expected([
+ [
+ null
+ ],
+ [],
+ ...
+ [
+ 4,
+ 6,
+ 7
+ ],
+ [
+ 2,
+ 3
+ ]
+])expected";
auto array = ArrayFromJSON(list_type, "[[null], [], null, [4, 6, 7], [2,
3]]");
- CheckArray(*array, {0, 10}, ex);
- CheckArray(*array, {2, 10}, ex_2);
- CheckStream(*array, {0, 1}, ex_3);
Review comment:
Did the `CheckStream` check just disappear? (does it matter?)
--
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]
