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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
