Conversation
tustvold
added
api-change
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
This looks good to me, although it will need to wait for the next breaking release as it is a breaking change
| DataType::List(Arc::new(Field::new_list_field(DataType::UInt64, false))); | ||
| let list_data_type_string = list_data_type.to_string(); | ||
| let expected_string = "List(UInt64)"; | ||
| assert_eq!(list_data_type_string, expected_string); |
There was a problem hiding this comment.
Perhaps we could get a test of a nested list as well
|
Thanks @tustvold , I have made the changes as you suggested. Could you please help review it again? |
arrow-schema/src/datatype.rs
Outdated
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "{self:?}") | ||
| match self { | ||
| DataType::List(field) => { |
There was a problem hiding this comment.
We also need to print the field name here if it isn't the default of item
There was a problem hiding this comment.
Right -- so if field.name() != "item" perhaps it could look like
List(Int8;N, field='foo')
Likewise, if there is metadata I think it should also be displayed like
List(Int8;N, field='foo', metadata)
|
The Arrow docstring on datatype says the Display should be also reversible - look at the code of datatype_parse it doesn't seem like lists were parseable even before, but maybe it'd be good to add as part of this PR given the new format should be easier to parse? :) |
arrow-schema/src/datatype.rs
Outdated
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "{self:?}") | ||
| match self { | ||
| DataType::List(field) => { |
There was a problem hiding this comment.
maybe add the same for LargeList and FixedSizeList?
There was a problem hiding this comment.
Thanks @Blizzara for your advice, that's good idea!
|
Thanks @irenjj for working on this. I agree with @Blizzara. The display result should be reversible. I prefer to format the DataType::List(field) => {
write!(f, "List({})", field.data_type())
}The result would be something like arrow-rs/arrow-schema/src/datatype_parse.rs Line 604 in 7905545 |
|
Thanks for working on this! Note that a lot of error messages actually uses the |
|
@irenjj do you plan to work on this any more? If not, perhaps someone else wants to pick it up to get it ready to go? |
I've been a bit busy lately, if anyone is interested, feel free to take it.:) |
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
|
I'll take a stab at it in |
6 participants
Which issue does this PR close?
Closes #7048
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?