[Rust] pitfall in xxx_equal functions

[asfimport]

Functions pub(super) fn xxxx_equal in rust/src/array/equal/ MUST fail if called directly instead of from equal.  boolean.rs as  example:

lhs_is_null || (lhs_is_null == rhs_is_null)
 && equal_bits(...)

 
The bug is not seen in tests at present because equal shortcuts the condition: zero nulls v.s. nulls
 
To verify this problem , adds a new test in mod.rs
 

#[test]
fn test_direct_call_boolean_equal() {
    let a = BooleanArray::from(vec![Some(true)]).data();
    let b = BooleanArray::from(vec![Some(false)]).data();
    let lhs = a.as_ref();
    let rhs = b.as_ref();
    let lhs_nulls = lhs.null_buffer();
    let rhs_nulls = rhs.null_buffer();
    assert_eq!(boolean::boolean_equal(lhs, rhs, lhs_nulls, rhs_nulls, 0, 0, lhs.len()), false);
}

Reporter: Qingyou Meng / @mqy

Related issues:

• [Rust] array::transform::tests::test_struct failed (relates to)

_{Note: This issue was originally created as ARROW-11160. Please see the migration documentation for further details.}

[asfimport]

Qingyou Meng / @mqy:
I tried to make some modifications to show possible problems, See

master...mqy:array-equal

[~nevime]  what do you think about this?

[asfimport]

Neville Dipale / @nevi-me:
@jorgecarleitao  any thoughts about this? I'll have a look in some detail

[asfimport]

Jorge Leitão / @jorgecarleitao:
Hi.

I am sorry, I can't understand what the issue is:

These functions are private implementations, which is why

1. they are marked as pub(super)
2. they are not exposed in mod.rs
3. there is a single public function on the whole equal package, equal.

I can't see how it is a problem to have private functions that require specific conditions to return a useful result.

Secondly, the test above passes on my computer, so I can't reproduce it :/

Thirdly, we can find the following comment in mod.rs:

// these methods assume the same type, len and null count.
// For this reason, they are not exposed and are instead used
// to build the generic functions below (`equal_range` and `equal`).
use boolean::boolean_equal;
...

I.e. IMO we are doing everything right:

1. keep them as implementation details
2. expose only what needs to be exposed (and test it accordingly)
3. document the conditions that need to be fulfilled for the functions to be useful

[asfimport]

Qingyou Meng / @mqy:
@jorgecarleitao  thanks for taking time to reply!

Firstly, the test code had wrong data in array a, fixed, my bad! This test fails on arrow master.

#[test]
fn test_direct_call_boolean_equal() {
    let a = BooleanArray::from(vec![None]).data();
    let b = BooleanArray::from(vec![Some(false)]).data();
    let lhs = a.as_ref();
    let rhs = b.as_ref();
    let lhs_nulls = lhs.null_buffer();
    let rhs_nulls = rhs.null_buffer();
    assert_eq!(boolean::boolean_equal(lhs, rhs, lhs_nulls, rhs_nulls, 0, 0, lhs.len()), false);
}

Secondly, I totally agree with you about private functions and the comment in mod.rs, that's great!

Thirdly,  it may be less error-prune to update condition from:

lhs_is_null || (lhs_is_null == rhs_is_null) && non_null_equal()

to

lhs_is_null == rhs_is_null && (lhs_is_null || non_null_equal())

I re-pushed master...mqy:array-equal  with several files changed, to demonstrating the failures at:

array::transform::tests::test_struct

Perhaps this is a hint to possible hidden bug somewhere, for example MutableArrayData::extend()? I'm sorry for taking your time again.