Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Standardize column alias behavior ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@282 PS7, Line 282: * Substitute exprs of the form "<number>" with the corresponding Update this comment to describe our new alias/ordinal substitution behavior. Also mention pre-and post conditions with respect to the state of 'exprs', i.e. the passed 'exprs' may or may not be analyze, but the 'exprs' are all analyzed after this function regardless of whether substitution was performed. http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@548 PS7, Line 548: havingClause_; > should I use havingClause_.clone() here? To simplify and be more internally consistent I think we should use substituteOrdinalsAliases(). I think we had a subtle bug here because of not using substituteOrdinalsAliases(). Mmbiguous aliases will not be reported and we will simply substitute one of them - which is wrong and inconsistent with our behavior for ORDER/GROUP BY. Can you confirm this bug and add a test? It's unfortunate that most other DBs are already inconsistent with each other. I feel that allowing ordinals in HAVING is consistent with allowing aliases/ordinals in GROUP BY, even if some other databases do not allow that. At least our behavior will be internally consistent, easy to explain, and very close to the behavior of PostgresSQL. http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@985 PS7, Line 985: // IMPALA-5191: In GROUP BY, HAVING, ORDER BY, aliases and ordinals must only be Now that aliases and ordinals are treated consistently, can you combine the ordinal tests above with these tests? Might be able to condense them with a loop or similar. http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@996 PS7, Line 996: + "group by nb having nb"); you can leave out group by http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1007 PS7, Line 1007: // select count(*) a ... GROUP BY, ORDER BY, HAVING Comment doesn't add much since we can see that from the test query test. A semantic description is more helpful, i.e. Alias referring to aggregation output in GROUP BY... http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1019 PS7, Line 1019: // select sum(id) over(order by id) a ... GROUP BY, ORDER BY, HAVING Alias referring to analytic output in GROUP BY ... -- To view, visit http://gerrit.cloudera.org:8080/8801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8 Gerrit-Change-Number: 8801 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 15 Dec 2017 17:22:42 +0000 Gerrit-HasComments: Yes
