-
Notifications
You must be signed in to change notification settings - Fork 834
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
Add Field::new_list_field
and improve DataType::new_list
docs
#4627
Conversation
76f4fe3
to
7fb3d98
Compare
arrow-schema/src/field.rs
Outdated
/// Field::new("item", DataType::Int32, true), | ||
/// Field::new_list_item(DataType::Int32, true) |
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.
It does occur to me that this is now longer 😅
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.
But it is more explict / discoverable
Maybe Field::new_list_field
? or Field::new_list
?
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 don't feel strongly, I personally don't see the issue with "item" but we're going to have to agree to disagree there 😅
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 guess new_list_field
? As this is to create a field.
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.
Great idea -- done in 76b1e70
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.
Not sure about new_list_item, docs on DataType::new_list look good
I'll give this PR a few more days to see if anyone else has thoughts / comments |
Co-authored-by: Liang-Chi Hsieh <[email protected]>
Co-authored-by: Liang-Chi Hsieh <[email protected]>
How should we proceed with this one? |
I will polish it up and get it merged |
Field::new_list_item
and improve DataType::new_list
docsField::new_list_field
and improve DataType::new_list
docs
FYI here is a bug in DataFusion related to not following the conventions apache/datafusion#9441 (and hence another reason in my mind that this API has value) |
Which issue does this PR close?
Re #4544
Rationale for this change
As described on #4561 (comment) the API for creating List fields is somewhat confusing and hiding details of doing so may make it more so.
What changes are included in this PR?
Field::new_list_item
Field::new_list_field
DataType::new_list
docsAre there any user-facing changes?
Yes a new API for constructing
Field
s and better docs