siddharthteotia commented on a change in pull request #7972:
URL: https://github.com/apache/pinot/pull/7972#discussion_r780712377
##########
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:
If possible, I would like to avoid additional pass over
`pinotQuery.getSelectList()`. How about we change code in lines 157 to 163 in
the following way to achieve this ?
```
boolean groupByListEmpty = pinotQuery.getGroupByList().isEmpty() ||
pinotQuery.getGroupByList() == null;
for (Expression selectExpression : pinotQuery.getSelectList()) {
if (isAggregateExpression(selectExpression)) {
aggregateExprCount++;
}
else if (!groupByListEmpty ) {
if (expressionOutsideGroupByList(selectExpression,
groupByExprs)) {
throw new SqlCompilationException(
"'" + RequestUtils.prettyPrint(selectExpression) + "'
should appear in GROUP BY clause.");
}
} else if (expressionContainsColumn(expr) && aggregateExprCount > 0) {
throw new SqlCompilationException(
"Non-aggregate and Aggregate expressions can't
co-exist without GROUP BY clause");
}
}
```
Let's also remove code between lines 149 and 152
Also I feel that we don't need to explicitly check
`expressionContainsColumn(expr)`. If the expression in the `selectList` is not
an aggregate expression, then it could either be a non-aggregate
literal/identifier/function expression. In any case, if the `aggregateExprCount
> 0`, we know there is a combination of non-aggregate and aggregate expression
together in the selectList without GROUP BY clause which is what we want to
find out and raise error
So the last else if condition can simply be `else if (aggregateExprCount >
0)`
--
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
