ankitsultana commented on code in PR #13792:
URL: https://github.com/apache/pinot/pull/13792#discussion_r1722347973
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/NonAggregationGroupByToDistinctQueryRewriter.java:
##########
@@ -41,7 +40,7 @@
* SELECT col1 AS c1 FROM foo GROUP BY col1 --> SELECT DISTINCT col1 AS c1
FROM foo
* SELECT col1, col1 AS c1, col2 FROM foo GROUP BY col1, col2 --> SELECT
DISTINCT col1, col1 AS ci, col2 FROM foo
*
- * Unsupported queries:
+ * Not rewritten queries:
Review Comment:
@tarun11Mavani : Are we sure we are only allowing the valid queries to pass
through here?
Invalid queries such as the following should continue to be disallowed.
`SELECT concat(col1, someOtherCol) from table GROUP BY col1, col2`.
cc: @vvivekiyer
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/NonAggregationGroupByToDistinctQueryRewriter.java:
##########
@@ -85,9 +85,7 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
pinotQuery.setGroupByList(null);
return pinotQuery;
} else {
- throw new SqlCompilationException(String.format(
- "For non-aggregation group-by query, select expression set and
group-by expression set should be the same. "
- + "Found select: %s, group-by: %s", selectExpressions,
groupByExpressions));
+ return pinotQuery;
Review Comment:
@tarun11Mavani : what is the behavior for queries that reference a column
not in the grouping set? Say
```
SELECT someCol FROM table GROUP BY col1, col2
```
Ideally we should fail such queries in the broker itself.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
