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 9: (11 comments) http://gerrit.cloudera.org:8080/#/c/8801/8/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/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@273 PS8, Line 273: * also refers to a select-list expression. > Let's inline this check into the single caller. Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@283 PS8, Line 283: Returns clone > "corresponding" is not really explained, I think we should explain ordinal I rewrote the whole comment. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@285 PS8, Line 285: */ > The returned expr is analyzed regardless of whether substitution was perfor Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@288 PS8, Line 288: Expr substituteExpr = trySubstituteOrdinal(expr, errorPrefix, analyzer); > substituteOrdinalOrAlias() Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@290 PS8, Line 290: if (ambiguousAliasList_.contains(expr)) { > Let's move this after attempting to substitute ordinals to make the flow cl Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@291 PS8, Line 291: throw new AnalysisException("Column '" + expr.toSql() + > Integrate this into the function comment. Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@296 PS8, Line 296: } else { > Slightly easier for me to read/reason with inverted logic, i.e. Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@314 PS8, Line 314: } > substituteOrdinalsAndAliases() Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@316 PS8, Line 316: > Less code to use a loop over indexes: Done http://gerrit.cloudera.org:8080/#/c/8801/8/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/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@987 PS8, Line 987: // Ambiguous alias in HAVING > Let's move this complex test to the end. Easier to follow the tests if we s Done http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1041 PS8, Line 1041: "GROUP BY expression must not contain analytic expressions: " + > Weird error. GROUPBY+HAVING come before analytics so the error should be "H 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: 9 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 09 Jan 2018 17:52:08 +0000 Gerrit-HasComments: Yes
