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, as this isn't a distinction found in the arrow spec. 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