Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8801 )
Change subject: IMPALA-5191: Standardize column alias behavior ...................................................................... Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/8801/6/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/6/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@289 PS6, Line 289: Analyzer analyzer, boolean substituteChildren) throws AnalysisException { * Adding this new flag here and in the Expr substitute() functions is a pretty invasive change for this simple fix. * We only ever call this substituteOrdinalsAliases() with substituteChildren==false because that's what the new behavior or, so the existence of this flag is even confusing * This function is the only place where we call the Expr substitute functions with substituteChildren==false, so modifying the Expr APIs does not make sense to me * The simpler alternative is to only substitute exprs that are a SlotRef, i.e. in L305 you can continue if the expr is not a SlotRef, otherwise substitute just as before (i.e. the fix for this JIRA can be a one-line change) * Please remove the substituteChildren flag here and elsewhere because that the change is too invasive for a simple change http://gerrit.cloudera.org:8080/#/c/8801/6/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/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@956 PS6, Line 956: AnalyzesOk("with w_test as (select '1' as one, 2 as two, '3' as three) " These tests have nothing to do with analytic exprs, so they don't belong in TestAnalyticExprs()? I think it's best to rename TestOrdinals() to TestAliasesAndOrdinals() and consolidate the relevant tests in there. http://gerrit.cloudera.org:8080/#/c/8801/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@961 PS6, Line 961: AnalyzesOk("select int_col / 2 as x from functional.alltypes group by x"); Would be good to add tests like these if not already present: * select count(*) a from t group by a - should error * select count(*) a from t having a - should error * select count(*) > 10 a from t having a - should work * select count(*) a from t order by a - should work * select sum() over(order by id) a from t group by a - should error * select sum() over(order by id) a from t order by a - should work ok Etc. You get the idea. -- 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: 6 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: Fri, 15 Dec 2017 01:40:41 +0000 Gerrit-HasComments: Yes
