walterddr commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r934734852
##########
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:
yes. I definitely understand this. since `count(*)` by definition counts
number of row since an entire row cannot be null; and `count(col)` only counts
for non-null.
however since the multi-stage engine executes a logical planner after
semantic parsing. `*` will always be resolved into a full list of table schema
(and in the case of count(*), `*` will be dropped as the logical count doesn't
take argument. see
https://github.com/apache/calcite/blob/b9c2099ea92a575084b55a206efc5dd341c0df62/core/src/main/java/org/apache/calcite/sql/SqlSyntax.java#L43-L47
so this is really the compatibility situation
- since pinot current SQL parser only takes it to the semantic level it
doesn't parse out the `*`, thus `*` remains a operand.
- but logically `*` means no argument in logical plan, thus in multi-stage
engine it doesn't have argument.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
