Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Standardize column alias behavior ...................................................................... Patch Set 8: (13 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: protected void checkForAmbiguousAliases(Expr expr, String errorPrefix) Let's inline this check into the single caller. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@283 PS8, Line 283: corresponding "corresponding" is not really explained, I think we should explain ordinal and alias substitution a little more. Returns the substitution of top-level integer NumericLiteral with the corresponding select-list expr http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@285 PS8, Line 285: * 'expr' is analyzed in this function regardless of whether The returned expr is analyzed regardless of whether substitution was performed. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@288 PS8, Line 288: protected Expr substituteOrdinalAlias(Expr expr, String errorPrefix, substituteOrdinalOrAlias() http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@290 PS8, Line 290: checkForAmbiguousAliases(expr, errorPrefix); Let's move this after attempting to substitute ordinals to make the flow clearer. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@291 PS8, Line 291: // We can substitute either by ordinal or by alias. Integrate this into the function comment. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@296 PS8, Line 296: if (substituteExpr == null) { Slightly easier for me to read/reason with inverted logic, i.e. if (substituteExpr != null) return substituteExpr; if (expr instanceof SlotRef) ... http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@314 PS8, Line 314: protected void substituteOrdinalsAliases(List<Expr> exprs, String errorPrefix, substituteOrdinalsAndAliases() http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@316 PS8, Line 316: ListIterator<Expr> i = exprs.listIterator(); Less code to use a loop over indexes: for (int i = 0; ...) { exprs.set(i, substitute(...)); } http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8801/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@548 PS7, Line 548: substituteOrdi > substituteOrdinalsAliases() takes a list of Exprs as param, while we only h Thanks for confirming and fixing! http://gerrit.cloudera.org:8080/#/c/8801/7/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/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@985 PS7, Line 985: // IMPALA-5191: In GROUP BY, HAVING, ORDER BY, aliases and ordinals must only be > I found it cumbersome to combine the tests, because some of them pass, some That's fine, maybe it's not worth consolidating the tests. I'll think more carefully about it. Good catch with the weird ordinal substitution. It's great that we keep finding/fixing more edge cases! To address it I think we should change SelectStmt.rewriteExprs() to not accept an expr rewrite in the GROUP BY, HAVING, and ORDER BY clauses if the original expression was not a NumericLiteral eligible to be substituted, but the rewritten expression is a NumericLiteral eligible for ordinal substitution. Here's an example that demonstrates inconsistent behavior in the GROUP BY clause: select int_col, count(*) from functional.alltypes group by 1*2, int_col; The query throws an error with enable_expr_rewrites=true, but works with enable_expr_rewrites=false What do you think? 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: AnalyzesOk("with w_test as (select '1' as one, 2 as two, '3' as three) " Let's move this complex test to the end. Easier to follow the tests if we start with the simple cases first. nit: Let's be consistent about multi-line strings, i.e. put the "+" in the preceding line since that's the prevalent style. http://gerrit.cloudera.org:8080/#/c/8801/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1041 PS8, Line 1041: AnalysisError("select sum(id) over(order by id) a from functional.alltypes having a", Weird error. GROUPBY+HAVING come before analytics so the error should be "HAVING clause must not contain analytic expressions". We can fix this by doing checkReturnsBool() after checking for analytics in analyzeAggregation(). Probably we should check for subqueries and analytics after substituting aliases and ordinals for consistency. -- 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: 8 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: Mon, 08 Jan 2018 20:38:54 +0000 Gerrit-HasComments: Yes
