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 4: (7 comments) Thanks for the 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! You're welcome! 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 Done 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? Extended 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? Added a comment. 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. Done 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 Done 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? Done -- 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: Wed, 13 Dec 2017 16:10:54 +0000 Gerrit-HasComments: Yes