Refine documentation to Array::is_null
#4838
Conversation
arrow-array/src/array/mod.rs
Outdated
| /// let array = NullArray::new(1); | ||
| /// assert_eq!(array.is_logical_null(0), true); | ||
| /// ``` | ||
| fn is_logical_null(&self, index: usize) -> bool { |
There was a problem hiding this comment.
I think at the very least we should provide an efficient implementation of this, instead of computing logical_nulls which could be very expensive.
In general I am really not a fan of adding this method, it is a fairly major potential performance footgun
| /// | ||
| /// // NullArrays do not have a validity mask | ||
| /// let array = NullArray::new(1); | ||
| /// assert_eq!(array.is_null(0), false); | ||
| /// ``` | ||
| fn is_null(&self, index: usize) -> bool { |
There was a problem hiding this comment.
I've said this before but I think from a user PoV, having is_null and is_logical_null is confusing as hell. Which NULL is is_null?! Yeah, historically this is the physical null but do most users really care about the physical repr.? I would argue that at least this method should be called is_physical_null to force users to think about what kind of null they want, instead of tricking them into using the wrong implicit default for their use case.
There was a problem hiding this comment.
Are you suggesting that is_null should always return logical nullability? What about for RunArray where this would have O(log(n)) complexity? What about null_count? The only consistent thing I can see is to only ever return physical nullability...
There was a problem hiding this comment.
I suggest that is_null should be renamed to is_physical_null (potentially w/ a soft deprecation period) to avoid that users accidentally pick the wrong method.
You make a good point regarding null_count. My argument would be: rename that one as well, to physical_null_count. Then it's clear to which semantic you're referring to.
|
@tustvold and I talked about this earlier today and what I suggest is:
|
alamb
changed the title
Array::is_null
Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
* Add documentation and Array::is_logical_null * Remove code change, refine comments * fix docs * Apply suggestions from code review Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> * Fix link formatting --------- Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
Which issue does this PR close?
Closes #4835
Rationale for this change
The fact that
NullArray::is_null()returns false is both consistent with the technical definition of physical/logical nulls, but also deeply confusing to a casual user, as explained on #4835I don't think the implications of logical vs physical nullability are well understood by the arrow user community (and to be honest I am not sure they should in most cases).
Thus helping them find the right API for what they want to do would be incel
What changes are included in this PR?
NullArraycase (the one where I think it is the most deeply confusing, even though this does potentially apply toDictionaryArrayandRunArrayArray::is_logical_nullthat returns the the logical nullability (mostly as a way to document the behavior and save downstream crates from having to handleNullArrayspecially)Are there any user-facing changes?
New function and improved documentation
Questions
Array::is_logical_nullArray::is_logical_validto mirrorArray::is_valid?