llama90 commented on code in PR #39192:
URL: https://github.com/apache/arrow/pull/39192#discussion_r1477007607
##########
cpp/src/arrow/scalar.cc:
##########
@@ -884,9 +997,11 @@ std::string Scalar::ToString() const {
return dict_scalar->value.dictionary->ToString() + "[" +
dict_scalar->value.index->ToString() + "]";
}
- auto maybe_repr = CastTo(utf8());
+
+ auto maybe_repr = Cast(CastToExplicitScalar(), utf8());
Review Comment:
By referencing your comment, I fixed it.
##########
cpp/src/arrow/scalar.cc:
##########
@@ -884,9 +997,11 @@ std::string Scalar::ToString() const {
return dict_scalar->value.dictionary->ToString() + "[" +
dict_scalar->value.index->ToString() + "]";
}
- auto maybe_repr = CastTo(utf8());
+
+ auto maybe_repr = Cast(CastToExplicitScalar(), utf8());
Review Comment:
@kou Hello, It's not complete yet, but do you have any better ideas about
this part?
Additionally, the issues related to the failure of this workflow include the
following items, and all seem to be associated with `ToString`:
- Replace from the legacy `CastTo` to the new `Cast` within `ToString`
- Improve some output formats (for `List`) for `ToString`
```
Value of: scalar.ToString()
Expected: (has substring "item: int16") and (ends with "[1, 2, null]")
Actual: "[\n [\n 1,\n 2,\n null\n ]\n]"
```
- Fix the GLib test cases related to this
Would it be better to handle these in a single PR?
Thank you as always.
##########
cpp/src/arrow/scalar.cc:
##########
@@ -884,9 +885,24 @@ std::string Scalar::ToString() const {
return dict_scalar->value.dictionary->ToString() + "[" +
dict_scalar->value.index->ToString() + "]";
}
- auto maybe_repr = CastTo(utf8());
+
+ if (type->id() == Type::LIST || type->id() == Type::LARGE_LIST ||
+ type->id() == Type::LIST_VIEW || type->id() == Type::LARGE_LIST_VIEW ||
+ type->id() == Type::FIXED_SIZE_LIST) {
+ auto list_scalar = checked_cast<const BaseListScalar*>(this);
+ return list_scalar->value->ToString();
Review Comment:
I am currently facing a dilemma regarding the output formatting. It has
become challenging to handle it in the same manner as before.
Here are the approaches I'm considering:
1. proceed with the current output format in `ToString`, then create an
issue to add a feature similar to `pretty_print.cc`. However, I'm concerned
about the impact on languages that are binding to the C++ code.
2. make `ToString` more extensible. How should we go about this?
I would greatly appreciate any guidance or advice on how to proceed.
cc @kou @bkietz
##########
cpp/src/arrow/scalar.cc:
##########
@@ -884,9 +885,24 @@ std::string Scalar::ToString() const {
return dict_scalar->value.dictionary->ToString() + "[" +
dict_scalar->value.index->ToString() + "]";
}
- auto maybe_repr = CastTo(utf8());
+
+ if (type->id() == Type::LIST || type->id() == Type::LARGE_LIST ||
+ type->id() == Type::LIST_VIEW || type->id() == Type::LARGE_LIST_VIEW ||
+ type->id() == Type::FIXED_SIZE_LIST) {
+ auto list_scalar = checked_cast<const BaseListScalar*>(this);
+ return list_scalar->value->ToString();
+ }
+
+ if (type->id() == Type::RUN_END_ENCODED) {
+ auto ree_scalar = checked_cast<const RunEndEncodedScalar*>(this);
+ return ree_scalar->value->ToString();
Review Comment:
same as `list`
--
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]
