siddharthteotia commented on a change in pull request #4535: Implement DISTINCT clause URL: https://github.com/apache/incubator-pinot/pull/4535#discussion_r320624737
########## File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/OutputColumnListAstNode.java ########## @@ -41,8 +43,87 @@ public void addChild(AstNode childNode) { } } + /** + * Check for following style PQLs and raise syntax error + * + * These 4 queries are not valid SQL queries as well so PQL won't support them too + * (1) SELECT sum(col1), min(col2), DISTINCT(col3, col4) + * (2) SELECT col1, col2, DISTINCT(col3) FROM foo + * (3) SELECT DISTINCT(col1, col2), DISTINCT(col3) FROM foo + * (4) SELECT timeConvert(DaysSinceEpoch,'DAYS','SECONDS'), DISTINCT(DaysSinceEpoch) FROM foo + * + * These 3 queries are either both selection and aggregation query + * or the query does not make sense from result point of view (like 6) + * (5) SELECT DISTINCT(col1), col2, col3 FROM foo + * (6) SELECT DISTINCT(col1), sum(col3), min(col4) FROM foo + * (7) SELECT DISTINCT(DaysSinceEpoch), timeConvert(DaysSinceEpoch,'DAYS','SECONDS') FROM foo + * + * SQL versions of the above queries: + * + * (1) SELECT sum(col1), min(col2), DISTINCT col3, col4 + * (2) SELECT col1, col2, DISTINCT col3 FROM foo + * (3) SELECT DISTINCT col1, col2, DISTINCT col3 FROM foo + * (4) SELECT timeConvert(DaysSinceEpoch,'DAYS','SECONDS'), DISTINCT DaysSinceEpoch FROM foo + * + * 1, 2, 3 and 4 will still not be supported in compliance with SQL + * + * (5) SELECT DISTINCT col1, col2, col3 FROM foo + * will be supported as it effectively becomes a multi column distinct + * + * (6) SELECT DISTINCT col1, sum(col3), min(col4) FROM foo + * although a valid SQL syntax for multi column distinct, if we decide to support + * them, we will have to do sum and min as transform functions which is not the case today. + * In any case, not a helpful query. + * + * (7) SELECT DISTINCT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS') FROM foo + * again a valid SQL syntax for multi column distinct, we can support this since timeConvert + * is a valid supported transform function. + */ + private void validate() { + boolean identifierPresent = false; + boolean distinctPresent = false; + boolean functionPresent = false; + if (hasChildren()) { + for (AstNode child : getChildren()) { + if (child instanceof OutputColumnAstNode) { Review comment: I don't think so. In fact it can only be of type OutputColumnAstNode which is an AstNode ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org