Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Behavior of column aliases should be more standard conforming ...................................................................... Patch Set 3: (7 comments) 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, "Standardize column alias behavior" 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. 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 substituted." 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. 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 parameter is "substituteChildren", or something similar. What do you think? 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? 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 another case where we have "group by x having x" and x is a boolean? (It won't work if it's not a boolean, right?) -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 12 Dec 2017 01:11:02 +0000 Gerrit-HasComments: Yes
