Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 )
Change subject: IMPALA-7844: HAVING clause cannot support ordinals ...................................................................... Patch Set 4: (10 comments) Revised based on review comments. Rebased on master. Thanks for the suggestion that we want to wait until a major release to disable ordinals. Fortunately, it was easy to add a constant that controls the feature: set it to enable an ordinal in HAVING by default, to avoid breaking anything. At the major release, the constant can transform into a feature flag that can be enabled in the unlikely case anyone actually uses ordinals in HAVING. Tests are conditional on the feature setting. The refactoring is still needed for other work, so please do review the updated code. 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 Done http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG@21 PS4, Line 21: than > then Done http://gerrit.cloudera.org:8080/#/c/11955/4//COMMIT_MSG@24 PS4, Line 24: unusal > typo Done 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 Done 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 I agree that this code is confusing. Here's my understanding: This bit of code replaces alias and ordinals (both of which are references to a SELECT list item). In SQL, both are semi-equivalent ways to refer to result set columns. For historical reasons, we allow alias only as the only item in an expression. (Works for ORDER BY, not for HAVING.) This code replaces the ordinal/alias with the result set *expression*, which is surprising. Most systems replace it with a reference to that column (as "slot ref" in Impala terminology.) Later, we use an "smap" (substation map) to match the expression with a result set expression. While odd, this does unify the cases of a) ordinal, b) column alias, and c) repeating the result set expression. The main change here is to recognize that ordinals work in lists (ORDER BY) but not expressions (HAVING). 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 Done 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? Done http://gerrit.cloudera.org:8080/#/c/11955/4/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/11955/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@549 PS4, Line 549: in the list > in the predicate? Done 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? Sorry, -ve? 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 Analys Revised. Given the hesitancy to change the behavior now, preserved original behavior, gated by a constant. Used that constant to choose the new tests for HAVING <int> above, or the "legacy" tests here. -- 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 <par0...@yahoo.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Dec 2018 02:04:57 +0000 Gerrit-HasComments: Yes
