crepererum commented on code in PR #4838:
URL: https://github.com/apache/arrow-rs/pull/4838#discussion_r1330002357
##########
arrow-array/src/array/mod.rs:
##########
@@ -184,41 +184,76 @@ pub trait Array: std::fmt::Debug + Send + Sync {
///
/// In most cases this will be the same as [`Array::nulls`], except for:
///
- /// * DictionaryArray where [`DictionaryArray::values`] contains nulls
- /// * RunArray where [`RunArray::values`] contains nulls
- /// * NullArray where all indices are nulls
+ /// * ['DictionaryArray`] where [`DictionaryArray::values`] contains nulls
+ /// * [`RunArray`] where [`RunArray::values`] contains nulls
+ /// * [`NullArray`] where all indices are nulls
///
/// In these cases a logical [`NullBuffer`] will be computed, encoding the
logical nullability
/// of these arrays, beyond what is encoded in [`Array::nulls`]
fn logical_nulls(&self) -> Option<NullBuffer> {
self.nulls().cloned()
}
- /// Returns whether the element at `index` is null.
- /// When using this function on a slice, the index is relative to the
slice.
+ /// Returns whether the element at `index` is null, according to
[`Array::nulls`]
///
- /// Note: this method returns the physical nullability, i.e. that encoded
in [`Array::nulls`]
- /// see [`Array::logical_nulls`] for logical nullability
+ /// # Notes
+ /// 1. This method returns false for [`NullArray`] as explained below. See
+ /// [`Self::is_logical_null`] for an implementation that returns the
logical
+ /// null value.
+ ///
+ /// 2. When using this function on a slice, the index is relative to the
slice.
+ ///
+ /// 3. This method returns the value in the **physical** validity bitmap
for an element,
+ /// as returned by [`Array::nulls`]. If there is no validity bitmap,
returns `true`.
+ /// See [`Array::logical_nulls`] for logical nullability
///
/// # Example:
///
/// ```
- /// use arrow_array::{Array, Int32Array};
+ /// use arrow_array::{Array, Int32Array, NullArray};
///
/// let array = Int32Array::from(vec![Some(1), None]);
///
/// assert_eq!(array.is_null(0), false);
/// assert_eq!(array.is_null(1), true);
+ ///
+ /// // 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 {
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
