Specialize ValueCountAggregator depending on type of ValuesSource

[polyfractal]

The value_count aggregation uses the bytesValue() of its ValuesSource. This is simple/convenient since all the different types implement it, and ValueCount doesn't need to specify what kind of ValuesSource it expects.

But the downside is that when value_count is counting up float/doubles or longs we are converting the numerics to Strings first.

This is pretty slow, and creates a ton of temporary garbage due to all the string manipulation:

Since value_count doesn't actually need the value, just the count of values in the field, it wouldn't be too difficult to specialize the aggregator so that slightly different versions are used for each type of ValuesSource. Then it can call the appropriate method (doubleValues(), etc) and avoid all the string casting.

[elasticmachine]

Pinging @elastic/es-analytics-geo

[amishra123]

hi there, I would like to work on this :).

[polyfractal]

@amishra123 sure, have at it :)

Fair warning, I only skimmed over the various parts of code when opening this issue, so it may be trickier than I thought once you get deep into the guts of it.

[talevy]

hi @amishra123, are you still interested in tackling this?

[xjtushilei]

I have encountered the same problem.
The valuecount performance is too bad.
Can I comment out this part of the code? Especially to sort the array, etc.

[polyfractal]

Hi @xjtushilei, afraid it's not that easy :) The data is actually already pre-sorted (most doc values are sorted when they are indexed, or if they are a single-valued field there is no sorting at all).

There's not an easy/simple way to fix this, since we'll need specialized version for each data type. Fetching the bytes value (and resulting string casting) is simple because it applies to all data types. But to make it more efficient, we need the aggregator to create a specialized internal aggregator that knows how to use strings vs longs vs geo points, etc.

[xjtushilei]

Thanks for your answer. @polyfractal
Valuecount is just counting the number, and doing something that I think is meaningless, maybe I don't understand his role. I want to say a phenomenon, I found that ValueCountAgg is slower than SumAgg, which is beyond my common sense. So valueCount can be made much simpler.

The optimization I want to do is

1. Remove the sort. I found out why sorting is also needed in SortedBinaryDocValues.advanceExact.I have timed this part of the code, which consumes a lot of time.

                grow();
                for (int i = 0; i < count; ++i) {
                    final CharSequence s = list.get(i);
                    values[i].copyChars(s);
                }
                sort();

I think the sort may not be used.
2. Only take out the data for counting up, do not need to convert to string

Is this feasible?

[polyfractal]

Ah, I see the sort you are referring to. Yes, the purpose of this issue is to remove this sorting behavior, which I agree is unnecessary. :)

It's a side effect of how the aggregation framework is structured, and not as simple as just removing the sort() though. This part of the code is used by other aggregations which rely on the sorted behavior. It isn't needed for value_count, but is needed for other aggs.

The reason this is happening is because value_count accepts any data type (ValuesSourceType.ANY). The only method that all the different ValuesSources share is bytesValue(), which returns or converts the values into a byte representation. For some fields (like strings) the data is already in a bytes format and no work is done. Other fields like numerics are converted to strings first, which then needs to be re-sorted.

Which is all redundant work for value_count.

So the purpose of this ticket is to introduce new, specialized forms of ValueCount for each data type. These could be either distinct aggregators like how we do it with terms aggs (e.g. LongValueCountAggregator, StringValueCountAggregator, etc), or a specialized leaf collector inside the single aggregator. In either case, these specializations would only work with a certain ValuesSourceType, and could thus call the "native" data access method (longValues(), doubleValues()) and avoid the need to fall back on the generic bytesValue() method.

I hope that helps! The answer is basically: yes, the sorting and string conversion is totally unnecessary for value_count, but it'll take a little bit of work to change this behavior because of how the agg framework is structured.

[xjtushilei]

Thanks for your help. I will try it.

[xjtushilei]

#54854 related