[C++] Filter kernel for MonthDayNanoInterval array returning garbage values

[jorisvandenbossche]

Example with latest pyarrow:

>>> import pyarrow as pa
>>> pa.__version__
Out[30]: '15.0.0.dev436+gac50918a6.d20240117'
>>> import pyarrow.compute as pc

>>> arr = pa.array([pa.MonthDayNano((1, 1, 1)), pa.MonthDayNano((2, 2, 2))])
>>> arr
<pyarrow.lib.MonthDayNanoIntervalArray object at 0x7f8e89147220>
[
 1M1d1ns,
 2M2d2ns
]
>>> pc.filter(arr, pa.array([True, False]))
Out[36]: 
<pyarrow.lib.MonthDayNanoIntervalArray object at 0x7f8e89e24580>
[
 3M0d0ns # <-- this value didn't even exist in the original array
]

>>> arr = pa.array([pa.MonthDayNano((1, 1, 1)), None])
>>> arr
<pyarrow.lib.MonthDayNanoIntervalArray object at 0x7f8e89e24820>
[
 1M1d1ns,
 null
]
>>> pc.filter(arr, pa.array([True, False]))
<pyarrow.lib.MonthDayNanoIntervalArray object at 0x7f8e88e4eda0>
[
 null # <-- this should have selected the first element of the array, but returning null instead
]

[jorisvandenbossche]

Note that also take is failing (in the end filter is translated into an equivalent take, I suppose, or at least sharing code):

>>> arr.take([0])
<pyarrow.lib.MonthDayNanoIntervalArray object at 0x7f8e88f93520>
[
 null
]

[jorisvandenbossche]

The interval types are considered as a primitive type, so that's the reason this kernel is not raising a not-implemented error (while there is no explicit support for intervals).

However, it seems this PrimitiveTakeExec isn't set up to handle the larger bit width of internvals (128 for MonthDayNano):

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

Lines 575 to 595 in 3b73f43

	switch (bit_width) {
	case 1:
	TakeIndexDispatch<BooleanTakeImpl>(values, indices, out_arr);
	break;
	case 8:
	TakeIndexDispatch<PrimitiveTakeImpl, int8_t>(values, indices, out_arr);
	break;
	case 16:
	TakeIndexDispatch<PrimitiveTakeImpl, int16_t>(values, indices, out_arr);
	break;
	case 32:
	TakeIndexDispatch<PrimitiveTakeImpl, int32_t>(values, indices, out_arr);
	break;
	case 64:
	TakeIndexDispatch<PrimitiveTakeImpl, int64_t>(values, indices, out_arr);
	break;
	default:
	DCHECK(false) << "Invalid values byte width";
	break;
	}
	return Status::OK();

So we are just allocating some values for the output, but then not setting anything (and also not raising an error for this, because it's only a debug check ..)

[jorisvandenbossche]

We should maybe also consider to stop treating MonthDayNano interval automatically as primitive type (is_primitive in type_traits.h returns True for all interval types)

[pitrou]

At least, the DCHECK should be turned into an actual error return.

[pitrou]

We should maybe also consider to stop treating MonthDayNano interval automatically as primitive type

Well, it is a primitive type. This is just a bug in the take and filter code that it doesn't properly handle more primitive type widths.

[WillAyd]

Out of curiosity how does Arrow define "primitive"?

[pitrou]

Unfortunately there's no rigorous conceptual definition, it's more of an adhoc category. Roughly, everything that's a number, a boolean or a temporal is considered primitive, except decimals for some reason.