Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 )
Change subject: IMPALA-7844: HAVING clause cannot support ordinals ...................................................................... Patch Set 4: (9 comments) Minor comments. Core of the patch lgtm. Zoltan, could you take a pass too? I see that you made some significant refactoring here in IMPALA-5191. http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG@21 PS4, Line 21: opeators typo http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG@21 PS4, Line 21: than then http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG@24 PS4, Line 24: unusal typo http://gerrit.cloudera.org:8080/#/c/11955/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/11955/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@329 PS4, Line 329: resoluton typo http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@336 PS4, Line 336: Substitutes Not sure if the substitution is happening here. As I understand it, we just resolve the ordinal/alias and return an analyzed expr clone which the callers can substitute anywhere. http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@353 PS4, Line 353: allowOrdinal mention this in the method doc http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@389 PS4, Line 389: Analyze it so all expressions exit : // this method analyzed. Isn't this happening L372? http://gerrit.cloudera.org:8080/#/c/11955/4/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/11955/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1074 PS4, Line 1074: "if(true, 7, int_col)"); Test -ve ordinal values? http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1124 PS4, Line 1124: @Ignore("IMPALA-7844: Ordinals not supported in HAVING") Don't think this is the right way. Instead, convert them into proper AnalysisError() calls? -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 12 Dec 2018 06:15:48 +0000 Gerrit-HasComments: Yes
