Improve UnionArray::is_nullable #6540
Conversation
| @@ -1181,4 +1181,18 @@ mod tests { | |||
| .collect(); | |||
| assert_eq!(values, vec![Some(vec![1, 2, 3]), None, Some(vec![4, 5, 6])]) | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
I don't understand how this test connects to the code change. All the union members are nullable, no?
There was a problem hiding this comment.
This is a test from the linked issue #6538.
There was a problem hiding this comment.
I see, but it doesn't answer my question.
There was a problem hiding this comment.
The ListArray's field is not nullable, so doesn't permit a nullable child. Without the change in this PR the test fails
There was a problem hiding this comment.
Could we also have a more direct check for the code change, i.e. one that directly asserts UnionArray::is_nullable (in the respective module)? I'm fine with keeping the actual regression test.
There was a problem hiding this comment.
I verified that this test fails without the code change:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Non-nullable field of ListArray \"element\" cannot contain nulls")
thread 'array::list_array::tests::test_nullable_union' panicked at arrow-array/src/array/list_array.rs:228:54:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Non-nullable field of ListArray \"element\" cannot contain nulls")
stack backtrace:
There was a problem hiding this comment.
I also added a specific test in e78c87b
There was a problem hiding this comment.
Thank you @tustvold , @crepererum and @mbrobbel and @findepi
| @@ -1181,4 +1181,18 @@ mod tests { | |||
| .collect(); | |||
| assert_eq!(values, vec![Some(vec![1, 2, 3]), None, Some(vec![4, 5, 6])]) | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
I verified that this test fails without the code change:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Non-nullable field of ListArray \"element\" cannot contain nulls")
thread 'array::list_array::tests::test_nullable_union' panicked at arrow-array/src/array/list_array.rs:228:54:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Non-nullable field of ListArray \"element\" cannot contain nulls")
stack backtrace:
| @@ -1181,4 +1181,18 @@ mod tests { | |||
| .collect(); | |||
| assert_eq!(values, vec![Some(vec![1, 2, 3]), None, Some(vec![4, 5, 6])]) | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
I also added a specific test in e78c87b
|
Thank you everyone |
Which issue does this PR close?
Relates to #6538.
Rationale for this change
Always returning true is overly pessimistic, and whilst this is still not accurate, it is at least slightly more accurate
What changes are included in this PR?
Are there any user-facing changes?