Add ListBuilder::with_field to support non nullable list fields (#5330) #5331
Conversation
| /// | ||
| /// By default a nullable field is created with the name `item` | ||
| /// | ||
| /// Note: [`Self::finish`] and [`Self::finish_cloned`] will panic if the provided data type does not match `T` |
There was a problem hiding this comment.
The ergonomics of this aren't ideal, but aside from adding a data_type method to ArrayBuilder I'm not sure of a way around this.
| } | ||
| } | ||
|
|
||
| /// Override the field passed to [`GenericListArray::new`] |
There was a problem hiding this comment.
This avoids duplicating the docs about what the implications of different nullability options are
| let array_data = unsafe { array_data_builder.build_unchecked() }; | ||
| let field = match &self.field { | ||
| Some(f) => f.clone(), | ||
| None => Arc::new(Field::new("item", values.data_type().clone(), true)), |
There was a problem hiding this comment.
| None => Arc::new(Field::new("item", values.data_type().clone(), true)), | |
| None => Arc::new(Field::new_list_field(values.data_type().clone(), true)), |
Minor nitpick, but noticed this function for creating the "default" list item field.
There was a problem hiding this comment.
This would create a field of DataType::List which isn't what we want here
There was a problem hiding this comment.
Oh, I must have looked at the wrong thing, the docs say:
assert_eq!(
Field::new("item", DataType::Int32, true),
Field::new_list_field(DataType::Int32, true)
);https://docs.rs/arrow-schema/latest/arrow_schema/struct.Field.html#method.new_list_field
There was a problem hiding this comment.
Ahh, could it be that you've mistaken new_list_field for new_list? https://docs.rs/arrow-schema/latest/arrow_schema/struct.Field.html#method.new_list
There was a problem hiding this comment.
Oh I didn't realise the field version got added in the end 😅. I'm old fashioned and prefer the explicit form, but good suggestion
There was a problem hiding this comment.
Yeah, I think there are two camps in terms of "do I know / want to know what the name of the anonymous field are in the list"
Specifically,
- if you are familiar with the convention, then it is better to have the code explcit.
- If you don't know the
"item"convention having a function hide it is easier to understand
alamb
changed the title
| let array_data = unsafe { array_data_builder.build_unchecked() }; | ||
| let field = match &self.field { | ||
| Some(f) => f.clone(), | ||
| None => Arc::new(Field::new("item", values.data_type().clone(), true)), |
There was a problem hiding this comment.
Yeah, I think there are two camps in terms of "do I know / want to know what the name of the anonymous field are in the list"
Specifically,
- if you are familiar with the convention, then it is better to have the code explcit.
- If you don't know the
"item"convention having a function hide it is easier to understand
|
|
||
| #[test] | ||
| fn test_non_nullable_list() { | ||
| let field = Arc::new(Field::new("item", DataType::Int32, false)); |
There was a problem hiding this comment.
Would it make sense to also add a test for a field that is not named "item"?
| fn test_non_nullable_list() { | ||
| let field = Arc::new(Field::new("item", DataType::Int32, false)); | ||
| fn test_with_field() { | ||
| let field = Arc::new(Field::new("bar", DataType::Int32, false)); |
Which issue does this PR close?
Closes #5330
Rationale for this change
This lets users override the
Fieldused byListBuilder, allowing overriding the nullability, name, and other metadata. It also switches over to using the saferListArrayconstructors.What changes are included in this PR?
Are there any user-facing changes?