You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/08/12 13:56:34 UTC

[GitHub] [arrow-rs] tustvold opened a new pull request, #4691: Logical Nullability

tustvold opened a new pull request, #4691:
URL: https://github.com/apache/arrow-rs/pull/4691

   # Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   Closes #4690
   Closes #4689
   Closes #4688
   Closes #2687
   Closes #4616 
   Relates to #4565
   
   # Rationale for this change
    
   <!--
   Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
   Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
   -->
   
   Various types have nullability that is not accurately reflected in their reported NullBuffer, this causes incorrect behaviour for various kernels that have special handling of the null buffers.
   
   # What changes are included in this PR?
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   Adds two methods to Array:
   
   * Array::logical_nulls - which computes the logical null mask
   * Array::is_nullable - which only returns false if an array cannot contain nulls
   
   # Are there any user-facing changes?
   
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!---
   If there are any breaking changes to public APIs, please add the `breaking change` label.
   -->
   


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

[GitHub] [arrow-rs] tustvold merged pull request #4691: Improve `Array` Logical Nullability

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691


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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317385


##########
arrow-array/src/array/mod.rs:
##########
@@ -226,6 +253,19 @@ pub trait Array: std::fmt::Debug + Send + Sync {
         self.nulls().map(|n| n.null_count()).unwrap_or_default()
     }
 
+    /// Returns `false` if the array is guaranteed to not contain any logical nulls
+    ///
+    /// In general this will be equivalent to `Array::null_count() != 0` but may differ in the
+    /// presence of logical nullability, see [`Array::logical_nulls`].
+    ///
+    /// Implementations will return `true` unless they can cheaply prove no logical nulls
+    /// are present. For example a [`DictionaryArray`] with nullable values will still return true,
+    /// even if the nulls present in [`DictionaryArray::values`] are not referenced by any key,
+    /// and therefore would not appear in [`Array::logical_nulls`].
+    fn is_nullable(&self) -> bool {

Review Comment:
   I debated calling this method is_required instead, but figured it was better to stick with consistent terminology, and this matches the method on `Field`



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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317464


##########
arrow-array/src/array/null_array.rs:
##########
@@ -107,22 +109,12 @@ impl Array for NullArray {
         None
     }
 
-    /// Returns whether the element at `index` is null.
-    /// All elements of a `NullArray` are always null.
-    fn is_null(&self, _index: usize) -> bool {

Review Comment:
   This is a breaking change, although I expect the impact to minimal. I debated making is_null and friends always return logical nullability, but this would have been inconsistent with ArrayData and generally seemed like it might just cause more confusion



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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317328


##########
arrow-arith/src/boolean.rs:
##########
@@ -311,11 +311,7 @@ pub fn not(left: &BooleanArray) -> Result<BooleanArray, ArrowError> {
 /// assert_eq!(a_is_null, BooleanArray::from(vec![false, false, true]));
 /// ```
 pub fn is_null(input: &dyn Array) -> Result<BooleanArray, ArrowError> {
-    let values = match input.nulls() {

Review Comment:
   We no longer need to special case NullArray, whilst also now correctly handling DictionaryArray with value nulls



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

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

Posted by "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, 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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1293170672


##########
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:
   I think `Array::nulls` should be deprecated and a non-deprecated version `physical_nulls` should be introduced. Users will misuse `nulls` because:
   
   - most people will read over the small note within the doc string
   - `nulls` is easier to find than `logical_nulls`
   - there are two options of "which null", and we offer an implicit default (because it is not prefixed with a semantic) that is clearly the wrong one for many use cases



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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317585


##########
arrow-ord/src/sort.rs:
##########
@@ -4016,4 +4016,30 @@ mod tests {
             vec![None, None, None, Some(5.1), Some(5.1), Some(3.0), Some(1.2)],
         );
     }
+
+    #[test]
+    fn test_lexicographic_comparator_null_dict_values() {

Review Comment:
   Test for #2687



##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
@@ -1760,6 +1759,35 @@ mod tests {
             max_rep_level: 2,
         };
 
-        assert_eq!(list_level, &expected_level);
+        assert_eq!(levels[0], expected_level);
+    }
+
+    #[test]
+    fn test_null_dictionary_values() {

Review Comment:
   Test for #4690



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

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

Posted by "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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292321612


##########
arrow-array/src/array/mod.rs:
##########
@@ -277,6 +317,10 @@ impl Array for ArrayRef {
         self.as_ref().nulls()
     }
 
+    fn logical_nulls(&self) -> Option<NullBuffer> {

Review Comment:
   I considered having this eagerly constructed and stored on the arrays, but I felt this would be more surprising as it would make the arrays diverge from the arrow specification. Whilst this does raise the prospect of computing this NullBuffer multiple times, the cases where this is required are fairly rare. We can always optimise in future should this become a problem



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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317530


##########
arrow-ord/src/comparison.rs:
##########
@@ -6352,4 +6352,18 @@ mod tests {
             .to_string()
             .contains("Could not convert ToType with to_i128"));
     }
+
+    #[test]
+    #[cfg(feature = "dyn_cmp_dict")]
+    fn test_dictionary_nested_nulls() {

Review Comment:
   This is a test for #4688 



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

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

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4691:
URL: https://github.com/apache/arrow-rs/pull/4691#discussion_r1292317350


##########
arrow-array/src/array/dictionary_array.rs:
##########
@@ -1253,4 +1286,20 @@ mod tests {
             assert_eq!(v, expected, "{idx}");
         }
     }
+
+    #[test]
+    fn test_iterator_nulls() {

Review Comment:
   Test for #4616 



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