Improve `Array` Logical Nullability #4691

tustvold · 2023-08-12T13:56:32Z

Which issue does this PR close?

Closes #4690
Closes #4689
Closes #4688
Closes #2687
Closes #4616
Relates to #4565
Relates to #4835

Rationale for this change

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?

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

This logical null mask can then be used in the places where the logic relies on logical nullability

Are there any user-facing changes?

tustvold · 2023-08-12T13:57:39Z

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() {


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

tustvold · 2023-08-12T13:58:03Z

arrow-array/src/array/dictionary_array.rs

@@ -1253,4 +1286,20 @@ mod tests {
            assert_eq!(v, expected, "{idx}");
        }
    }
+
+    #[test]
+    fn test_iterator_nulls() {


Test for #4616

tustvold · 2023-08-12T13:58:30Z

arrow-array/src/array/mod.rs

+    /// 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 {


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

tustvold · 2023-08-12T13:59:15Z

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 {


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

tustvold · 2023-08-12T14:00:13Z

arrow-ord/src/comparison.rs

+
+    #[test]
+    #[cfg(feature = "dyn_cmp_dict")]
+    fn test_dictionary_nested_nulls() {


This is a test for #4688

tustvold · 2023-08-12T14:00:43Z

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() {


Test for #2687

tustvold · 2023-08-12T14:00:54Z

parquet/src/arrow/arrow_writer/levels.rs

+    }
+
+    #[test]
+    fn test_null_dictionary_values() {


Test for #4690

tustvold · 2023-08-12T14:07:49Z

arrow-array/src/array/mod.rs

@@ -277,6 +317,10 @@ impl Array for ArrayRef {
        self.as_ref().nulls()
    }

+    fn logical_nulls(&self) -> Option<NullBuffer> {


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

crepererum · 2023-08-14T08:42:28Z

arrow-array/src/array/mod.rs

    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:


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

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

tustvold added the api-change Changes to the arrow API label Aug 12, 2023

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Aug 12, 2023

tustvold commented Aug 12, 2023

View reviewed changes

parquet/src/arrow/arrow_writer/levels.rs

}

#[test]

fn test_null_dictionary_values() {

Copy link

Contributor Author

tustvold Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for #4690

tustvold force-pushed the logical-nullability branch from ab28cfe to 721d65e Compare August 12, 2023 14:03

Logical Nullability

55a420b

tustvold force-pushed the logical-nullability branch from 721d65e to 55a420b Compare August 12, 2023 14:04

tustvold commented Aug 12, 2023

View reviewed changes

alamb changed the title ~~Logical Nullability~~ Improve Array Logical Nullability Aug 13, 2023

alamb mentioned this pull request Aug 13, 2023

Cleanup DynComparator (#2654) #4687

Merged

crepererum reviewed Aug 14, 2023

View reviewed changes

crepererum approved these changes Aug 14, 2023

View reviewed changes

tustvold merged commit 979a070 into apache:master Aug 14, 2023
28 checks passed

tustvold mentioned this pull request Sep 18, 2023

NullArray::is_null() returns false incorrectly #4835

Closed

wjones127 mentioned this pull request Oct 11, 2023

chore: upgrade datafusion and arrow-rs lancedb/lance#1390

Closed

tustvold mentioned this pull request Oct 21, 2023

Add MultiPartStore (#4961) (#4608) #4971

Merged

alamb mentioned this pull request Dec 14, 2023

Avoid forced copy in Array::logical_nulls #5208

Open

tustvold mentioned this pull request Feb 24, 2024

Support Nested Arrays in Comparison Kernels #5426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Array` Logical Nullability #4691

Improve `Array` Logical Nullability #4691

tustvold commented Aug 12, 2023 •

edited by alamb

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

tustvold Aug 12, 2023

crepererum Aug 14, 2023

tustvold Aug 14, 2023 •

edited

Improve Array Logical Nullability #4691

Improve Array Logical Nullability #4691

Conversation

tustvold commented Aug 12, 2023 • edited by alamb

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Aug 14, 2023 • edited

Choose a reason for hiding this comment

Improve `Array` Logical Nullability #4691

Improve `Array` Logical Nullability #4691

tustvold commented Aug 12, 2023 •

edited by alamb

tustvold Aug 14, 2023 •

edited