walterddr commented on code in PR #10268:
URL: https://github.com/apache/pinot/pull/10268#discussion_r1106184694
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -50,7 +50,11 @@ public CountAggregationFunction(ExpressionContext
expression) {
public CountAggregationFunction(ExpressionContext expression, boolean
nullHandlingEnabled) {
super(expression);
// Consider null values only when null handling is enabled and function is
not COUNT(*)
- _nullHandlingEnabled = nullHandlingEnabled &&
!expression.getIdentifier().equals("*");
+ // Note COUNT on any literal gives same result as COUNT(*)
+ // So allow for identifiers that are not * and functions, disable for
literals and *
+ _nullHandlingEnabled = nullHandlingEnabled
+ && ((expression.getType() == ExpressionContext.Type.IDENTIFIER &&
!expression.getIdentifier().equals("*"))
+ || (expression.getType() == ExpressionContext.Type.FUNCTION));
Review Comment:
technically speaking in standardized SQL count(col) returns a number of
non-null value count for that column in a table.
- `count(*)` and `count(1)` (or count any non-null literal) are equivalent
- `count(col)` is technically not equivalent to `count(*)`
- when nullHandlingEnabled = false. it is equivalent to `count(*)`
- when nullHandlingEnabled = true. i am not sure if the current result
is correct (but this is out of scope of this PR)
so my suggestion is
```suggestion
|| (expression.getType() == ExpressionContext.Type.FUNCTION)
|| (expression.getType() == ExpressionContext.Type.LITERAL &&
expression.getLiteral().getValue() != null));
```
--
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]
