[GitHub] [druid] gianm commented on a change in pull request #11208: fix count and average SQL aggregators on constant virtual columns

[GitBox <gi...@apache.org>]

gianm commented on a change in pull request #11208:
URL: https://github.com/apache/druid/pull/11208#discussion_r627625346



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/CountSqlAggregator.java
##########
@@ -134,15 +134,17 @@ public Aggregation toDruidAggregation(
     } else {
       // Not COUNT(*), not distinct
       // COUNT(x) should count all non-null values of x.
-      return Aggregation.create(createCountAggregatorFactory(
+      AggregatorFactory theCount = createCountAggregatorFactory(

Review comment:
       How about making createCountAggregatorFactory return Aggregation instead of AggregatorFactory? It seems like that would be less error-prone. (Currently, since it returns an AggregatorFactory that might reference some virtual columns, all callers must check for that, which is easy to forget [hence this bug] and makes the callers more complex.)




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org