[C++] min/max aggregate for floating points can return values not contained in column

[rohanjain101]

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

>>> pa.compute.max(pa.array([float("nan"), float("nan")]))
<pyarrow.DoubleScalar: -inf>
>>> pa.compute.min(pa.array([float("nan"), float("nan")]))
<pyarrow.DoubleScalar: inf>
>>>

min/max returned values that are not in the column. Perhaps NaN should be returned instead?

Component(s)

C++

[pitrou]

That sounds like a reasonable suggestion. We should make sure the current behavior is not deliberate, though. cc @zanmato1984

[pitrou]

The current behavior has the nice property that it makes filtering using statistics easier than if NaN has to be handled explicitly.

[zanmato1984]

Despite whether we are doing so deliberately (most likely not), I don't think emitting -inf or inf is desirable for an all-NaN array. The proposed returning NaN sounds reasonable. I think current undesirable behavior is because we initialize our running min/max as -inf/inf. And by definition std::fmin/fmax(non-NaN, NaN) always emits non-NaN. I think I can work on a fix.

[pitrou]

As a data point, NumPy has a function nanmin that ignores NaN when computing an array's minimum, but it still returns NaN if no other values are encountered (it also emits a warning):

>>> np.nanmin(np.array([1.0, float('nan')]))
np.float64(1.0)
>>> np.nanmin(np.array([float('nan')]))
<ipython-input-17-324f73f42199>:1: RuntimeWarning: All-NaN slice encountered
  np.nanmin(np.array([float('nan')]))
np.float64(nan)

However, it also doesn't support an empty input array unless you pass an initial argument, and then...

>>> np.nanmin(np.array([]))
Traceback (most recent call last):
  ...
ValueError: zero-size array to reduction operation fmin which has no identity

>>> np.nanmin(np.array([]), initial=5.0)
np.float64(5.0)
>>> np.nanmin(np.array([float('nan')]), initial=5.0)
np.float64(5.0)

Arrow on the other hand returns Null if an empty array is given:

>>> pc.min(pa.array([], type=pa.float64()))
<pyarrow.DoubleScalar: None>

Thought @jorisvandenbossche @bkietz @westonpace @WillAyd ?

[zanmato1984]

From the implementation perspective, I think the cheapest and reasonable solution is that we do:

min([]) == null;
min([nan, nan, nan]) == nan;
min([nan, nan, 42.0)] == 42.0;

We can simply initialize the running min/max to nan (instead of -inf/inf) and leverage the nice property of fmin/fmax that for all two nans return nan, otherwise the non-nan. And allowing empty array and all-nan array, that would be otherwise considered invalid in NumPy, shouldn't be much of a harm perhaps?

[pitrou]

I would be ok with that. Let's perhaps wait for other opinions?

[zanmato1984]

Issue resolved by pull request 48459
#48459