[Python][C++] UnionArray with invalid data passes validation / leads to segfaults

[asfimport]

From the Python side, you can create an "invalid" UnionArray:

binary = pa.array([b'a', b'b', b'c', b'd'], type='binary') 
int64 = pa.array([1, 2, 3], type='int64') 
types = pa.array([0, 1, 0, 0, 2, 1, 0], type='int8')   # <- value of 2 is out of bound for number of childs
value_offsets = pa.array([0, 0, 2, 1, 1, 2, 3], type='int32')

a = pa.UnionArray.from_dense(types, value_offsets, [binary, int64])

Eg on conversion to python this leads to a segfault:

In [7]: a.to_pylist()
Segmentation fault (core dumped)

On the other hand, doing an explicit validation does not give an error:

In [8]: a.validate()

Should the validation raise errors for this case? (the C++ ValidateVisitor for UnionArray does nothing)

(so that this can be called from the Python API to avoid creating invalid arrays / segfaults there)

Reporter: Joris Van den Bossche / @jorisvandenbossche
Assignee: Antoine Pitrou / @pitrou

PRs and other links:

• GitHub Pull Request #5892

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

[asfimport]

Antoine Pitrou / @pitrou:
It's not obvious whether we want validate() to do a O(N) validation of the data.

@wesm

[asfimport]

Wes McKinney / @wesm:
Not obvious to me either. It seems like there is a dual need

• Checking very basic validity preconditions

• Actual data validation (boundschecking, checking monotonicity in the case of variable offsets, etc.)

  AFAICT we haven't really implemented much in the way of the latter. I think it'd be useful to have this in C++ but separate from the current Array::Validate I guess, and something that users can opt in to if they need to sanitize inputs

[asfimport]

Antoine Pitrou / @pitrou:
Yes, we may need a ValidateData method that's more thorough.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
The ListArray validation actually does something like the latter (it checks if all offsets are valid), so there is at least some precedent.

[asfimport]

Antoine Pitrou / @pitrou:
Hmm. Perhaps that validation can be moved to a separate method :-)
Then we'll have to make sure that all tests call the thorough validation method, rather than the light one.

[asfimport]

Antoine Pitrou / @pitrou:
Issue resolved by pull request 5892
#5892