jorisvandenbossche commented on code in PR #36496:
URL: https://github.com/apache/arrow/pull/36496#discussion_r1255837608
##########
cpp/src/arrow/extension/fixed_shape_tensor.h:
##########
@@ -62,6 +62,11 @@ class ARROW_EXPORT FixedShapeTensorType : public
ExtensionType {
std::string extension_name() const override { return
"arrow.fixed_shape_tensor"; }
+ std::string ToString() const override {
+ return "extension<" + this->extension_name() + "<type: " +
value_type_->ToString() +
+ ", " + this->Serialize() + ">>";
Review Comment:
> Just a nit - how about we have the whole string be JSON? .. just to make
potential parsing easier for users?
I wouldn't encourage people to start parsing the string (you have the type
object that you can inspect instead), and so I think we should optimize for
briefness + readability.
And indeed no other type uses this kind of json format, so as a starting
point I think we should try to be consistent with the formatting of existing
types.
So I would maybe even go for less json-like. Where this PR currently does:
```
fixed_shape_tensor<type: int32, {"shape":[2,2,3],"permutation":[0,2,1]}>
```
we could for example also go for
```
fixed_shape_tensor[type=int32, shape=[2,2,3], permutation=[0,2,1]]
# or
fixed_shape_tensor[int32, shape=[2,2,3], permutation=[0,2,1]]
# or
fixed_shape_tensor<int32>[shape=[2,2,3], permutation=[0,2,1]]
```
Generally we use `<..>` for types with a child field, and `[..]` for
parametrization (like `timestamp[us]`).
In this case we have a bit of both .. (if we consider it like a kind of
fixed size list, there is a child field that has a certain type, but it is also
parametrized further with shape and optional permutation/dims)
--
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]
