-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: parse DataType List, ListView, LargeList, LargeListView, FixedSizeList
#8649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5826fb5 to
e25e4a1
Compare
nullability and field for DataType::ListList, ListView, LargeList, LargeListView, FixedSizeList
|
|
||
| "nullable" => Token::Nullable, | ||
| "field" => Token::Field, | ||
| "x" => Token::X, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find a better name for x.
Using Time seems confuse with time related data types.
List, ListView, LargeList, LargeListView, FixedSizeListList, ListView, LargeList, LargeListView, FixedSizeList
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @dqkqd - this looks good to me
Co-authored-by: Andrew Lamb <[email protected]>
bc35472 to
b6d502c
Compare
|
Thank you @alamb for reviewing. |
|
Thanks again @dqkqd 🙏 |
|
While testing the next 57 release, I believe this PR has introduced several behavior changes / regresssions:
I have a fix for #8880 but ran out of time to look into #8883 more carefully . Does anyone have time to help check it out to unblock the release? |
# Which issue does this PR close? - Closes #8880 # Rationale for this change - #8649 change the default display for List and LargeList and FixedSizeList, and changed the parsing code to match. However, to maintain backwards compatibility we also need to support the old syntax too # What changes are included in this PR? WIP # Are these changes tested? WIP # Are there any user-facing changes?
|
Update here is that in order to maintain backwards compatibility we changed the behavior in So that For example
|
Which issue does this PR close?
parse_data_typeforList,ListView,LargeList,LargeListView,FixedSizeList,Union,Map,RunEndCoded. #8648.This PR only implements for list types to make review easier.
Rationale for this change
The format for
DataType::Listincludes:List(Int64): list not nullable.List(nullable Int64): list nullable.List(nullable Int64, field: 'foo'): list nullable with field.List(nullable Int64, metadata: {"foo1": "value1"}): list with metadata.(... The list goes on for
ListView,LargeList,LargeListView,FixedSizeList)parse_data_typecannot (or incorrectly) work on those data types listed above.What changes are included in this PR?
Token::...to support newDisplayformat for list types introduced in ImproveDisplayforDataType#8351(e.g.
FixedSizeList(5 x nullable Int64, field: 'foo').fn nullableto check whether nested data type is nullable.parse_single_quoted_stringandparse_list_field_nameto handlefield: 'foo'.Are these changes tested?
Yes. Added round trip tests.
Are there any user-facing changes?
Yes. This is related to #8351