You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jonahgao (via GitHub)" <gi...@apache.org> on 2024/04/06 16:16:53 UTC
[PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
jonahgao opened a new pull request, #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #9932.
## Rationale for this change
In a `GenericListArray`, the data in the `values` corresponding to a null list may not be empty; it can be any data.
```rust
pub struct GenericListArray<OffsetSize: OffsetSizeTrait> {
data_type: DataType,
nulls: Option<NullBuffer>,
values: ArrayRef,
value_offsets: OffsetBuffer<OffsetSize>,
}
```
For example, in the description of [this document](https://docs.rs/datafusion/latest/datafusion/common/arrow/array/struct.GenericListArray.html#representation), the value of the null list is `?`, it's not empty.
In this situation, merely checking the `values` is unsafe, we must obtain valid values based on the `value_offsets`.
## What changes are included in this PR?
- Fix bug in `unnest_generic_list`.
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975#issuecomment-2041782549
Nice fix! Thanks @jonahgao and @Jefffrey
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975#discussion_r1554728440
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -364,32 +364,31 @@ fn unnest_generic_list<T: OffsetSizeTrait, P: ArrowPrimitiveType<Native = T>>(
options: &UnnestOptions,
) -> Result<Arc<dyn Array + 'static>> {
let values = list_array.values();
- if list_array.null_count() == 0 || !options.preserve_nulls {
- Ok(values.clone())
- } else {
- let mut take_indicies_builder =
- PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
- let mut take_offset = 0;
+ if list_array.null_count() == 0 {
+ return Ok(values.clone());
+ }
- list_array.iter().for_each(|elem| match elem {
- Some(array) => {
- for i in 0..array.len() {
- // take_offset + i is always positive
- let take_index = P::Native::from_usize(take_offset + i).unwrap();
- take_indicies_builder.append_value(take_index);
- }
- take_offset += array.len();
- }
- None => {
+ let mut take_indicies_builder =
+ PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
+ let offsets = list_array.value_offsets();
+ for row in 0..list_array.len() {
+ if list_array.is_null(row) {
+ if options.preserve_nulls {
Review Comment:
Bit surprised that clippy didn't suggest collapsing these together? Either way, would it be slightly more efficient to put the `options.preserve_nulls` check first since this would never change per iteration?
Edit: ah nvm, it's because the outer one is an if else :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975#discussion_r1554728440
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -364,32 +364,31 @@ fn unnest_generic_list<T: OffsetSizeTrait, P: ArrowPrimitiveType<Native = T>>(
options: &UnnestOptions,
) -> Result<Arc<dyn Array + 'static>> {
let values = list_array.values();
- if list_array.null_count() == 0 || !options.preserve_nulls {
- Ok(values.clone())
- } else {
- let mut take_indicies_builder =
- PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
- let mut take_offset = 0;
+ if list_array.null_count() == 0 {
+ return Ok(values.clone());
+ }
- list_array.iter().for_each(|elem| match elem {
- Some(array) => {
- for i in 0..array.len() {
- // take_offset + i is always positive
- let take_index = P::Native::from_usize(take_offset + i).unwrap();
- take_indicies_builder.append_value(take_index);
- }
- take_offset += array.len();
- }
- None => {
+ let mut take_indicies_builder =
+ PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
+ let offsets = list_array.value_offsets();
+ for row in 0..list_array.len() {
+ if list_array.is_null(row) {
+ if options.preserve_nulls {
Review Comment:
~~Bit surprised that clippy didn't suggest collapsing these together? Either way, would it be slightly more efficient to put the `options.preserve_nulls` check first since this would never change per iteration?~~
Edit: ah nvm, it's because the outer one is an if else :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: improve `unnest_generic_list` handling of null list [arrow-datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #9975:
URL: https://github.com/apache/arrow-datafusion/pull/9975#discussion_r1554728440
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -364,32 +364,31 @@ fn unnest_generic_list<T: OffsetSizeTrait, P: ArrowPrimitiveType<Native = T>>(
options: &UnnestOptions,
) -> Result<Arc<dyn Array + 'static>> {
let values = list_array.values();
- if list_array.null_count() == 0 || !options.preserve_nulls {
- Ok(values.clone())
- } else {
- let mut take_indicies_builder =
- PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
- let mut take_offset = 0;
+ if list_array.null_count() == 0 {
+ return Ok(values.clone());
+ }
- list_array.iter().for_each(|elem| match elem {
- Some(array) => {
- for i in 0..array.len() {
- // take_offset + i is always positive
- let take_index = P::Native::from_usize(take_offset + i).unwrap();
- take_indicies_builder.append_value(take_index);
- }
- take_offset += array.len();
- }
- None => {
+ let mut take_indicies_builder =
+ PrimitiveArray::<P>::builder(values.len() + list_array.null_count());
+ let offsets = list_array.value_offsets();
+ for row in 0..list_array.len() {
+ if list_array.is_null(row) {
+ if options.preserve_nulls {
Review Comment:
Bit surprised that clippy didn't suggest collapsing these together? Either way, would it be slightly more efficient to put the `options.preserve_nulls` check first since this would never change per iteration?
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -596,3 +595,99 @@ where
Ok(RecordBatch::try_new(schema.clone(), arrays.to_vec())?)
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::{
+ array::AsArray,
+ datatypes::{DataType, Field},
+ };
+ use arrow_array::StringArray;
+ use arrow_buffer::{BooleanBufferBuilder, NullBuffer, OffsetBuffer};
+
+ // Create a ListArray with the following list values:
+ // [A, B, C], [], NULL, [D], NULL, [NULL, F]
Review Comment:
Nice tests :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org