Improve default implementation of Array::is_nullable #6721
Conversation
| @@ -315,8 +315,7 @@ pub trait Array: std::fmt::Debug + Send + Sync { | |||
| /// even if the nulls present in [`DictionaryArray::values`] are not referenced by any key, | |||
| /// and therefore would not appear in [`Array::logical_nulls`]. | |||
| fn is_nullable(&self) -> bool { | |||
| // TODO this is not necessarily perfect default implementation, since null_count() and logical_null_count() are not always equivalent | |||
| self.null_count() != 0 | |||
| self.logical_null_count() != 0 | |||
There was a problem hiding this comment.
Can we provide alternative implementations DictionaryArray and UnionArray as part of this, currently this will significantly regress performance for them
There was a problem hiding this comment.
the logical_null_count implementation should obviously be consistent with logical_nulls implementation, so let's do #6740 first
There was a problem hiding this comment.
i added explicit implementation of DictionaryArray::logical_null_count
i am not sure what to do about UnionArray. UnionArray::logical_nulls has non trivial logic. Can you please advise how this should be approached the best way? should I just copy that function?
cc @alamb
|
We should also update the doc comment for this as well, as currently it is highlighted as pessimisable |
findepi
force-pushed
the
findepi/improve-default-implementation-of-array-is-nullable-7e779e
branch
from
2d55feb
to
f8ed2cc
Compare
Since is_nullable returns `false` if the array is guaranteed to not contain any logical nulls, the correct default implementation could leverage `self.logical_null_count` more appropriately than `self.null_count`. To reduce chance of negative surprises in downstream projects, we could introduce the new behavior under `is_logically_nullable` name and deprecate `is_nullable` without changing it.
findepi
force-pushed
the
findepi/improve-default-implementation-of-array-is-nullable-7e779e
branch
from
f8ed2cc
to
9e6fbf8
Compare
Description
Improve default implementation of Array::is_nullable
Since is_nullable returns
falseif the array is guaranteed to not contain any logical nulls, the correct default implementation could leverageself.logical_null_countmore appropriately thanself.null_count.To reduce chance of negative surprises in downstream projects, we could introduce the new behavior under
is_logically_nullablename and deprecateis_nullablewithout changing it.Which issue does this PR close?
None
Rationale for this change
Fix technically incorrect default implementation of
Array::is_nullableWhat changes are included in this PR?
Change default implementation of
Array::is_nullable.Are there any user-facing changes?
No