kishoreg commented on a change in pull request #5259: Support Aggregation
functions with multiple arguments.
URL: https://github.com/apache/incubator-pinot/pull/5259#discussion_r409263873
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java
##########
@@ -42,79 +47,87 @@ private AggregationFunctionFactory() {
public static AggregationFunction getAggregationFunction(AggregationInfo
aggregationInfo,
@Nullable BrokerRequest brokerRequest) {
String functionName = aggregationInfo.getAggregationType();
+ String argumentsString =
AggregationFunctionUtils.getColumn(aggregationInfo);
+ List<String> arguments =
Arrays.asList(argumentsString.split(CompilerConstants.AGGREGATION_FUNCTION_ARG_SEPARATOR));
+
try {
String upperCaseFunctionName = functionName.toUpperCase();
if (upperCaseFunctionName.startsWith("PERCENTILE")) {
String remainingFunctionName = upperCaseFunctionName.substring(10);
+ List<String> args = new ArrayList<>(arguments);
if (remainingFunctionName.matches("\\d+")) {
// Percentile
- return new
PercentileAggregationFunction(parsePercentile(remainingFunctionName));
+ args.add(remainingFunctionName);
+ return new PercentileAggregationFunction(args);
} else if (remainingFunctionName.matches("EST\\d+")) {
// PercentileEst
- return new
PercentileEstAggregationFunction(parsePercentile(remainingFunctionName.substring(3)));
+ args.add(remainingFunctionName.substring(3));
+ return new PercentileEstAggregationFunction(args);
} else if (remainingFunctionName.matches("TDIGEST\\d+")) {
// PercentileTDigest
- return new
PercentileTDigestAggregationFunction(parsePercentile(remainingFunctionName.substring(7)));
+ args.add(remainingFunctionName.substring(7));
+ return new PercentileTDigestAggregationFunction(args);
} else if (remainingFunctionName.matches("\\d+MV")) {
// PercentileMV
- return new PercentileMVAggregationFunction(
- parsePercentile(remainingFunctionName.substring(0,
remainingFunctionName.length() - 2)));
+ args.add(remainingFunctionName.substring(0,
remainingFunctionName.length() - 2));
+ return new PercentileMVAggregationFunction(args);
} else if (remainingFunctionName.matches("EST\\d+MV")) {
// PercentileEstMV
- return new PercentileEstMVAggregationFunction(
- parsePercentile(remainingFunctionName.substring(3,
remainingFunctionName.length() - 2)));
+ args.add(remainingFunctionName.substring(3,
remainingFunctionName.length() - 2));
+ return new PercentileEstMVAggregationFunction(args);
} else if (remainingFunctionName.matches("TDIGEST\\d+MV")) {
// PercentileTDigestMV
- return new PercentileTDigestMVAggregationFunction(
- parsePercentile(remainingFunctionName.substring(7,
remainingFunctionName.length() - 2)));
+ args.add(remainingFunctionName.substring(7,
remainingFunctionName.length() - 2));
+ return new PercentileTDigestMVAggregationFunction(args);
} else {
throw new IllegalArgumentException();
}
} else {
+ String column = arguments.get(0);
switch (AggregationFunctionType.valueOf(upperCaseFunctionName)) {
case COUNT:
- return new CountAggregationFunction();
+ return new CountAggregationFunction(column);
Review comment:
should we add an init method instead that passes the arguments[] as it is,
this might allow us to plugin aggregation functions easily. we can do this in
the next PR as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
