[C++] Add ScalarAggregateOptions

[asfimport]

See discussion on #7308. MinMax has an option for null behavior, but Sum and Mean do not.

Reporter: Neal Richardson / @nealrichardson
Assignee: Rok Mihevc / @rok

PRs and other links:

• GitHub Pull Request #9758

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

[asfimport]

Krisztian Szucs / @kszucs:
I wonder if it would be better to add specific aggregate behavior in a more generic way.

It is certainly possible to add different flags for the aggregate functions and implement conditional aggregation within the kernel, but it requires different implementations for each kernel. How about having conditional aggregations by passing a boolean array as a second argument instead? Couple of examples:

Count(input)  // keep the default null behaviour
CountIf(input, mask=IsNull(input))
CountIf(input, mask=IsNotNull(input))
MeanIf(input, mask=Compare(input, other, EQUAL))

This way the user could add custom conditions without further kernel implementations, so it would make the aggregate kernels more flexible and maintainable. I see no performance problems with this approach either since we would process a bitmask similarly to the null checks.

References:

• in ibis all of the reduce operations accept a condition

• or the really handy aggregate combinators in clickhouse

  cc @wesm @nealrichardson @jorisvandenbossche

[asfimport]

Wes McKinney / @wesm:
@kszucs my desired approach for aggregating data with an unmaterialized filter is through use of selection vectors

This is already laying in wait in the ExecBatch data structure that is passed to the kernel's exec function

https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/exec.h#L176

[asfimport]

Krisztian Szucs / @kszucs:
Great, the intent looks the same. With that in progress do we still need to implement custom condition-like options for aggregations? I'd assume no.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
@kszucs regarding the original topic of this issue: for your suggestion to use a selection vector to deal with skipping nulls, that means that the kernels should change to not skip nulls by default, right? (as otherwise there is no way to get this behaviour, unless you let the caller check the null count in advance, but that would already be possible right now as well)

[asfimport]

Wes McKinney / @wesm:
I think there's still perhaps some value in short-circuiting when encountering non-zero null count when you don't want to skip nulls. This need not be mutually exclusive with the filter-query-processing stuff

[asfimport]

Krisztian Szucs / @kszucs:
@jorisvandenbossche Right.

[asfimport]

Krisztian Szucs / @kszucs:
If I understand correctly by not skipping nulls as the default having short-circuiting is just a matter of the exposed API.

[asfimport]

Wes McKinney / @wesm:
Right. Rather than debate this further, I'm going to assign this to myself if that's okay and I'll work up a patch in the next day or two

[asfimport]

Krisztian Szucs / @kszucs:
Thanks! I simply wasn't sure whether it worth to work on it if we have another solution in mind (if any).

[asfimport]

Wes McKinney / @wesm:
This also needs to have a "minimum # valid values" option to deal with the differing requirements in how functions like "sum" work on 0-length or all-null inputs, see discussion in

#7478 (review)

[asfimport]

Kouhei Sutou / @kou:
Issue resolved by pull request 9758
#9758