[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4691: Improve `Array` Logical Nullability

["tustvold (via GitHub)" <gi...@apache.org>]

tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1293197635


##########
arrow-array/src/array/mod.rs:
##########
@@ -173,12 +173,33 @@ pub trait Array: std::fmt::Debug + Send + Sync {
     /// ```
     fn offset(&self) -> usize;
 
-    /// Returns the null buffers of this array if any
+    /// Returns the null buffer of this array if any
+    ///
+    /// Note: some arrays can encode their nullability in their children, for example,
+    /// [`DictionaryArray::values`] values or [`RunArray::values`], or without a null buffer,
+    /// such as [`NullArray`]. Use [`Array::logical_nulls`] to obtain a computed mask encoding this
     fn nulls(&self) -> Option<&NullBuffer>;
 
+    /// Returns the logical null buffer of this array if any
+    ///
+    /// In most cases this will be the same as [`Array::nulls`], except for:

Review Comment:
   Whilst I do sort of agree, I'm not sure this wouldn't just cause more confusion. Would we also deprecate is_null, is_valid, etc... I think the current API hasn't been a major issue thus far, as the cases where it matters are rare.
   
   FWIW once we have StringView I think there is basically no reason to use DictionaryArray or RunArray



-- 
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