"or" kernel applied to slices may be invalid on later operations

[bjchambers]

I believe this is due to #807.

Specifically, or uses combine_option_bitmap which uses bit_slice to produce the resulting validity bitmap. But this can lead to cases where the null bitmap has an invalid length (corresponding to the original buffer) while the value array has a length corresponding to the slice.

This in turn can lead to later problems, such as the nullif kernel returning an Err (internally) when it tries to do a bitwise & between the validity bits and the value bits, since they aren't the same length. It then discards the results of this comparison (calling ok() instead of unwrap()) which leads to correct results.

(right.values() & &right_bitmap.bits).ok().map(|b| b.not())

I believe that either (1+2) or (3) should be done:

1. Replace the ok() with ? to propagate the error.
2. Change combine_option_bitmap should to always produce a bitmap of the requested length.
3. Change the logic within the nullif kernel to not fail if the right values and right bitmap have different lengths.

Any guidance on which direction the fix should go, and/or ideas on how to implement said fixes?

[bjchambers]

Failing test case.

I believe the assert_ne!(...) should be equal, and is why the later thing fails.

Note that it applies the nullif kernel to the same primitive and equal boolean arrays, but gets different results.

    #[test]
    fn test_bool_array_or_sliced() {
        let a: BooleanArray = vec![Some(true), Some(false), None]
            .into_iter()
            .cycle()
            .take(1024)
            .collect();

        let a = a.slice(0, 3);
        let a: &BooleanArray = a.as_any().downcast_ref().unwrap();

        let b = BooleanArray::from(vec![true, false, true]);

        let c = or(a, &b).unwrap();

        let expected = BooleanArray::from(vec![Some(true), Some(false), None]);

        assert_eq!(c, expected);

        // `nullif` kernel assumes that for a boolean array these are equal
        // since it applies the BitAnd operator between them. Either `logical_or`
        // or `nullif` is broken.
        assert_ne!(
            c.data().null_buffer().unwrap().len(),
            c.data().buffers()[0].len()
        );

        let p = crate::array::Int64Array::from(vec![5, 6, 7]);
        assert_eq!(
            crate::compute::nullif(&p, &expected).unwrap(),
            crate::compute::nullif(&p, &c).unwrap()
        );
    }

[jhorstmann]

Maybe a quick fix could be to use buffer_bin_and instead of the & operator in nullif, which allows passing in the array length.

[bjchambers]

It looks like that works, and fixes the behavior of nullif. Is the other behavior (the length of the null buffer differing from the length of the boolean data buffer) an error in general?

[jhorstmann]

Is the other behavior (the length of the null buffer differing from the length of the boolean data buffer) an error in general?

I don't think it is, as long as both contain enough space for the length of the array where they are used. In the case of primitive arrays, the length of the validity buffer (in bits) would usually also be larger than the number of elements in the data buffer because it gets rounded up to full bytes.

[tustvold]

Closed by a combination of #2940 which fixed the nullif kernel and #2693 which removed the implementations of std::ops::And, etc... for Buffer - as they were incredibly easy to accidentally misuse (not applying the offset and/or length correctly)