Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Standardize column alias behavior ...................................................................... Patch Set 3: (7 comments) Thanks for the comments! In the meantime I also realized that since I use trySubstitute() instead of substitute() for HAVING in PS3, I don't need to extend substitute() with a new flag, therefore far less code modifications are needed. http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-5191: Behavior of column aliases should be more standard conforming > You should try to use the imperative mood in the subject line. For example, Done http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG@11 PS3, Line 11: to be more standard conformant. > Add an example in the commit message of what is no longer allowed. Done http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@813 PS3, Line 813: it won't substitute > "If onlyTopLevel is true, child expressions of 'this' will not be substitut Done http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@833 PS3, Line 833: * the type of 'this'. > Add comment describing the new parameter. In the new patch set I don't change this method http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@836 PS3, Line 836: onlyTopLevel > I thought about this a little, and I think a more intuitive name for this p I agree, I use "substituteChildren" in the new patch set. http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@880 PS3, Line 880: onlyTopLevel > Isn't this always false? Yes. Now that I changed the flag's name to substituteChildren, it's always true. Therefore I pass 'true' as an argument in the new patch set. http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@961 PS3, Line 961: group by x" > It would be good to have a positive test with a HAVING clause. Maybe add an Added a new test. Yes, it only works with booleans. -- 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: 3 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: Tue, 12 Dec 2017 10:37:41 +0000 Gerrit-HasComments: Yes
