-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-10591: [Rust] Add support for StructArray to MutableArrayData #8850
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
| let data_type = arrays[0].data_type(); | ||
| use crate::datatypes::*; | ||
|
|
||
| // if any of the arrays has nulls, insertions from any array requires setting bits |
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.
Maybe this code could move to the struct branch in the childdata match below?
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.
This change is unrelated to this PR: it was a commit from a bug fix, that this PR was build on top of. It is required also for non-struct arrays :)
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.
Ah ok 👍 better to keep it then maybe in that case.
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.
Looks good to me
| /// are the arrays themselves and `true` if the user plans to call [MutableArrayData::extend_nulls]. | ||
| /// In other words, if `use_nulls` is `false`, calling [MutableArrayData::extend_nulls] should not be used. | ||
| pub fn new(arrays: Vec<&'a ArrayData>, use_nulls: bool, capacity: usize) -> Self { | ||
| pub fn new(arrays: Vec<&'a ArrayData>, mut use_nulls: bool, capacity: usize) -> Self { |
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.
#8848 appears to contain this same code -- though github seems to think merging will not be a problem
| let arrays = vec![array.as_ref()]; | ||
| let mut mutable = MutableArrayData::new(arrays, false, 0); | ||
|
|
||
| mutable.extend(0, 1, 3); |
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 wonder if ensuring the slice covers an actual string value would be useful (this slice just takes the two None, None values). It it it was like mutable.extend(0, 2, 4) that would also include Some("mark"). The same comment applies to test_struct_nulls as well
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 am sorry, I did not notice this comment. You are right, I will address it on a separate PR.
This allows
joins and any operator using theMutableArrayDatato use columns withStructArray.