Is DataType.BYTES control in aggregate() aggregateGroupBySV() aggregateGroupByMV() needed?

[damianoporta]

Hello, in different aggregators like the following:

• https://github.com/apache/incubator-pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunction.java#L100

• https://github.com/apache/incubator-pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MinMaxRangeAggregationFunction.java#L104

I see a "pseudo duplication" of the code because of the column DataType. But, is this really needed?
I think that serialization/deserialization is done in aggregationResultHolder object only, no?

[kishoreg]

@Jackie-Jiang @mayankshriv what do you think about this? Its not clear from the code why we are handling BYTES in AVG and MinMax

[Jackie-Jiang]

We allow serialized pre-aggregated BYTES for these aggregation functions so that users can pre-aggregate the records, or enable star-tree on these functions:

• AVG
• MINMAXRANGE
• DISTINCTCOUNTHLL
• PERCENTILEEST
• PERCENTILETDIGEST

[kishoreg]

so, this is not a must for other functions right?

[Jackie-Jiang]

It is required if these conditions are met:

• The intermediate result type is Object instead of Number or String
• User might want to pre-aggregate for the aggregation function

E.g. for DistinctCountAggregationFunction, even though the intermediate result type is IntOpenHashSet, we don't want user to pre-aggregate because the size of the pre-aggregated result is unbounded. In such case, we don't support BYTES type.

[kishoreg]

Not an issue