[C++] "case_when" test failure on random union inputs

[pitrou]

Describe the bug, including details regarding any error messages, version, and platform.

Enabling this commented out test in scalar_if_else_test_.cc produces test failures:

// TEST(TestCaseWhen, UnionBoolStringRandom) {
//   for (const auto& type : std::vector<std::shared_ptr<DataType>>{
//            sparse_union({field("a", boolean()), field("b", utf8())}, {2, 7}),
//            dense_union({field("a", boolean()), field("b", utf8())}, {2, 7})}) {
//     ARROW_SCOPED_TRACE(type->ToString());
//     TestCaseWhenRandom(type);
//   }
// }

Component(s)

C++

[pitrou]

cc @wjones127 @lidavidm

[pitrou]

(#15131 is needed to reproduce)

[zanmato1984]

Looked a bit. The error message is

'::arrow::internal::ValidateArrayFull(output)' failed with Invalid: Union value at position 0 has invalid type id 0

, assuming this is the error that this issue encountered.

The reason of this failure is:

1. The random array generation for union types doesn't take into account the type code specified in the UnionDataType object:

   arrow/cpp/src/arrow/testing/random.cc

   Line 897 in 0552217

   // Trivial type codes map

2. The function case_when executes fine because the type id generation is also trivial
   

   arrow/cpp/src/arrow/testing/random.cc

   Line 901 in 0552217

   // Generate array of type ids

3. Finally a full validation is conducted on the function output
   

   arrow/cpp/src/arrow/compute/kernels/test_util.cc

   Line 320 in 0552217

   ValidateOutputImpl(*output.array());

   
   , which checks if type ids in the buffer match the ones specified in the data type (and they don't)
   

   arrow/cpp/src/arrow/array/validate.cc

   Line 362 in 0552217

   return Status::Invalid("Union value at position ", i, " has invalid type id ",

I think I can take this and make random array generation respect the type ids specified in the data type, instead of generating trivial type ids.

[zanmato1984]

I found that we can bring the test back alive by simply removing passing customized type codes, drafted here: #39308
And I think it's acceptable for this specific case because the case is about testing case_when rather than the "type code mapping" thing for union types.
Of course this will leave the potential issue of mismatched type codes between in data type and actual data unchanged. So I'm also open to do a thorough change, i.e., making random union array generation respect the type ids specified in the data type.
Any comments will be helpful. Thanks.
cc @pitrou

[pitrou]

@zanmato1984 Thanks for the investigation! I think it would be better to make random union array generation conformant, indeed.

[zanmato1984]

@zanmato1984 Thanks for the investigation! I think it would be better to make random union array generation conformant, indeed.

Sure, will do. Thanks @pitrou !

[zanmato1984]

One quick question:
In order to generate conformant type ids for random union array, I'll have to pass either UnionDataType or type codes into the following functions:

arrow/cpp/src/arrow/testing/random.h

Lines 535 to 537 in 91b2243

	std::shared_ptr<Array> SparseUnion(const ArrayVector& fields, int64_t size,
	int64_t alignment = kDefaultBufferAlignment,
	MemoryPool* memory_pool = default_memory_pool());

arrow/cpp/src/arrow/testing/random.h

Lines 549 to 551 in 91b2243

	std::shared_ptr<Array> DenseUnion(const ArrayVector& fields, int64_t size,
	int64_t alignment = kDefaultBufferAlignment,
	MemoryPool* memory_pool = default_memory_pool());

However they seem to be public APIs, even though they belong to arrow-testing library. Do I have to make overloads of them or am I free to change the signatures?

I appreciate if you can enlighten me about this? Thanks @pitrou !

[pitrou]

arrow-testing is not really for public usage, so you can change those APIs.

[zanmato1984]

Just found that #36018 already addressed the aforementioned problem. I made a mistake, my local branch was too old.

Now this test case can be brought back alive without other changes, updated in #39308.

So sad. And my apology for the unnecessary trouble :-(