Jackie-Jiang commented on a change in pull request #4535: Implement DISTINCT
clause
URL: https://github.com/apache/incubator-pinot/pull/4535#discussion_r324929754
##########
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:
Can you add comments on that? Also, if StarAstNode exists, there should be
no other nodes based on the sql standard. We should have check on that
----------------------------------------------------------------
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
