[GitHub] [pinot] siddharthteotia 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>]

siddharthteotia commented on a change in pull request #7972:
URL: https://github.com/apache/pinot/pull/7972#discussion_r780023177



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -147,6 +147,9 @@ static void validate(PinotQuery pinotQuery)
   private static void validateGroupByClause(PinotQuery pinotQuery)
       throws SqlCompilationException {
     if (pinotQuery.getGroupByList() == null) {
+      if (containsAggregateAndColsInSelectClause(pinotQuery)) {
+        throw new SqlCompilationException("Columns and Aggregate functions can't co-exist without GROUP BY clause");
+      }
       return;
     }
     // Sanity check group by query: All non-aggregate expression in selection list should be also included in group

Review comment:
       Isn't this code block checking what this PR is trying to accomplish ? From the description, lines 157 to 162 check if the select expression is not an aggregate expression and also not a group by expression then raise error. So all queries like following should be caught here imo. 
   
   If these queries are not being caught in this code block, then I think we should fix this code instead of introducing another function to do the same check
   
   ```
   SELECT col1, col2, count(*) FROM foo GROUP BY col1 -- col2 is not present in group by
   SELECt col1, count(*) FROM foo -- col1 not present in group by
   ```




-- 
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