davecromberge commented on code in PR #14373:
URL: https://github.com/apache/pinot/pull/14373#discussion_r1856262413
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/aggregator/DistinctCountCPCSketchAggregator.java:
##########
@@ -30,7 +31,7 @@ public DistinctCountCPCSketchAggregator() {
}
@Override
- public Object aggregate(Object value1, Object value2) {
+ public Object aggregate(Object value1, Object value2, Map<String, String>
functionParameters) {
Review Comment:
Thanks for fleshing out your idea further @swaminathanmanish. I agree that
using a concrete type with given fields would generally lead to more readable
and maintainable code. My biggest concern is that not all value aggregators
would work off the same function parameters and thus we might need more
flexibility. Since we are not able to create a unified
AggregationFunctionParams type, we would need to use something generic like
Map<String, String>, JsonObject or Object which, while providing flexibility
does reduce readability.
For example, the theta sketch value aggregator might expect
`_sketchNominalEntries` to be set whereas the CPC or HLL function may expect a
different parameter which cannot be unified with that of Theta.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
