Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Standardize column alias behavior ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/8801/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8801/4//COMMIT_MSG@13 PS4, Line 13: === Allowed === Nice, thanks for the examples! http://gerrit.cloudera.org:8080/#/c/8801/4/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/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@813 PS4, Line 813: is false I think it's better to say what happens when it's true. (Right now there is a double negative in this sentence). http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@838 PS4, Line 838: return trySubstitute(smap, analyzer, preserveRootType, true); Maybe mention why this is always true in the method comment? http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@850 PS4, Line 850: result.add(e.trySubstitute(smap, analyzer, preserveRootTypes, true)); Maybe add a brief comment why it's always true? http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@869 PS4, Line 869: * If substituteChildren is false, child expressions of 'this' will not be substituted. Modify comment to remove the double negative. http://gerrit.cloudera.org:8080/#/c/8801/4/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/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@303 PS4, Line 303: substituteExpr = expr.trySubstitute(aliasSmap_, analyzer, false, substituteChildren); long line http://gerrit.cloudera.org:8080/#/c/8801/4/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/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@963 PS4, Line 963: bool_col maybe make this "not bool_col" so that it's an expression? -- 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: 4 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 21:53:12 +0000 Gerrit-HasComments: Yes
