mcvsubbu commented on a change in pull request #5259: Support Aggregation
functions with multiple arguments.
URL: https://github.com/apache/incubator-pinot/pull/5259#discussion_r409714931
##########
File path:
pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -232,50 +233,32 @@ private AggregationInfo buildAggregationInfo(Function
function) {
throw new Pql2CompilationException("Aggregation function expects non
null argument");
}
- String columnName;
+ String argumentString;
String functionName = function.getOperator();
- if
(functionName.equalsIgnoreCase(AggregationFunctionType.DISTINCT.getName())) {
- // DISTINCT can support multiple arguments
- if (operands.size() == 1) {
- // single column DISTINCT
- columnName = getColumnExpression(operands.get(0));
- } else {
- // multi column DISTINCT
- Set<String> expressions = new HashSet<>();
- StringBuilder sb = new StringBuilder();
- int numOperands = operands.size();
- for (int i = 0; i < numOperands; i++) {
- Expression expression = operands.get(i);
- String columnExpression = getColumnExpression(expression);
- if (expressions.add(columnExpression)) {
- // deduplicate the columns
- if (i != 0) {
- sb.append(FunctionCallAstNode.DISTINCT_MULTI_COLUMN_SEPARATOR);
- }
- sb.append(getColumnExpression(expression));
+ if
(functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
+ argumentString = "*";
+ } else {
+ Set<String> expressions = new HashSet<>();
+ StringBuilder sb = new StringBuilder();
+ int numOperands = operands.size();
+ for (int i = 0; i < numOperands; i++) {
+ Expression expression = operands.get(i);
+ String columnExpression = getColumnExpression(expression);
+ if (expressions.add(columnExpression)) {
+ // deduplicate the columns
+ if (i != 0) {
+ sb.append(CompilerConstants.AGGREGATION_FUNCTION_ARG_SEPARATOR);
}
+ sb.append(getColumnExpression(expression));
}
- columnName = sb.toString();
- }
- } else {
- // other aggregation functions support exactly one argument
- if (operands.size() != 1) {
- throw new Pql2CompilationException(
- "Aggregation function" + function.getOperator() + " expects 1
argument. found: " + operands);
- }
-
- if
(functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
- columnName = "*";
- } else {
- Expression functionParam = operands.get(0);
- columnName = getColumnExpression(functionParam);
}
+ argumentString = sb.toString();
}
AggregationInfo aggregationInfo = new AggregationInfo();
aggregationInfo.setAggregationType(functionName);
-
aggregationInfo.putToAggregationParams(FunctionCallAstNode.COLUMN_KEY_IN_AGGREGATION_INFO,
columnName);
+
aggregationInfo.putToAggregationParams(CompilerConstants.COLUMN_KEY_IN_AGGREGATION_INFO,
argumentString);
Review comment:
Perhaps we should introduce a map that specifies the number of arguments
aggregation function expects (if fixed)? We can then throw an error right here
for functions that cannot handle multiple arguments.
----------------------------------------------------------------
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]
