[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7972: Broker Side validation for the query with aggregation and col but without group by

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

Jackie-Jiang commented on a change in pull request #7972:
URL: https://github.com/apache/pinot/pull/7972#discussion_r784373637



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -167,6 +170,18 @@ private static void validateGroupByClause(PinotQuery pinotQuery)
     }
   }
 
+  private static boolean containsAggregateAndColsInSelectClause(PinotQuery pinotQuery) {
+    int aggregateExprCount = 0;
+    int identifierExprCount = 0;
+    for (Expression expr : pinotQuery.getSelectList()) {
+      if (isAggregateExpression(expr)) {
+        aggregateExprCount++;
+      } else if (expressionContainsColumn(expr)) {
+        identifierExprCount++;
+      }
+    }
+    return identifierExprCount > 0 && aggregateExprCount > 0;
+  }

Review comment:
       (minor, code format) Add empty line between functions

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -167,6 +170,18 @@ private static void validateGroupByClause(PinotQuery pinotQuery)
     }
   }
 
+  private static boolean containsAggregateAndColsInSelectClause(PinotQuery pinotQuery) {
+    int aggregateExprCount = 0;
+    int identifierExprCount = 0;
+    for (Expression expr : pinotQuery.getSelectList()) {
+      if (isAggregateExpression(expr)) {
+        aggregateExprCount++;
+      } else if (expressionContainsColumn(expr)) {

Review comment:
       I feel we can simplify the checks. Without group-by clause, the select expressions should be either all aggregations or no aggregations (I don't think we need to support mixing literals and aggregations)

##########
File path: pinot-common/src/test/resources/sql_queries.list
##########
@@ -820,8 +820,8 @@ select mapKey(mapField,k1) from baseballStats where mapKey(mapField,k1) = 'v1'
 SELECT count(c1), sum(c1), min(c1), max(c1), avg(c1), minmaxrange(c1), distinctcount(c1), distinctcounthll(c1) FROM foo
 SELECT distinctcountrawhll(c1), fasthll(c1), percentile90(c1), percentileest95(c1), percentiletdigest(c1, 99) FROM foo
 SELECT countmv(c1), summv(c1), minmv(c1), maxmv(c1), avgmv(c1), minmaxrangemv(c1), distinctcountmv(c1), distinctcounthllmv(c1) FROM foo
-SELECT distinctcountrawhllmv(c1), fasthllmv(c1), percentile90mv(c1), percentileest95mv(c1), percentiletdigestmv(c1, 99) FROM foo
+SELECT distinctcountrawhllmv(c1), percentile90mv(c1), percentileest95mv(c1), percentiletdigestmv(c1, 99) FROM foo

Review comment:
       (minor) Any specific reason changing these 2 queries?




-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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