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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to