nizarhejazi commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r934033762
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -81,11 +81,6 @@ public static ExpressionContext getExpression(Expression
thriftExpression) {
*/
public static FunctionContext getFunction(Function thriftFunction) {
String functionName = thriftFunction.getOperator();
- if
(functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
- // NOTE: COUNT always take one single argument "*"
- return new FunctionContext(FunctionContext.Type.AGGREGATION,
AggregationFunctionType.COUNT.getName(),
- new
ArrayList<>(Collections.singletonList(ExpressionContext.forIdentifier("*"))));
- }
Review Comment:
@walterddr In order to handle nulls in count function, we need to be able to
accept two syntaxes: `count(col)` and `count(*)`. We don't handle nulls when
the input is `count(*)` but we handle it for `count(col)` by not counting null
values in `col`.
I removed these lines and updated all calls of `CountAggregationFunction`
constructor to send the count expression including the constructor calls in
pinot-query-runtime. Please check `AggregateOperator.java` and
`AggregationFunctionFactory.java` files. Only pinot-query-runtime still don't
pass the argument passed in `CountAggregationFunction` constructor.
cc: @Jackie-Jiang
--
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
