Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15877 )
Change subject: IMPALA-452 Add support for string concatenation operator using || construct Separated "||" and "OR" into different tokens. -OR (KW_OR) remains the same. (it creates CompoundPredicate and expects two BOOLEAN operands) -|| (KW_LOGICAL_OR) creates CompoundVe ...................................................................... Patch Set 9: (4 comments) http://gerrit.cloudera.org:8080/#/c/15877/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15877/9//COMMIT_MSG@7 PS9, Line 7: IMPALA-452 Add support for string concatenation operator using || construct Separated "||" and "OR" into different tokens. commit message is still weirdly formatted. https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala has some guidelines: Finally, please write a good, clear commit message, with a short, descriptive title and a message that is exactly long enough to explain what the problem was, and how it was fixed. Each should have 72 or fewer characters if possible. The first line should have an empty line after it, and the first line should begin with the ticket(s) addressed, followed by a colon and a space: "IMPALA-1234: ". Docs-only commits should have [DOCS] after the ticket numbers, like "IMPALA-1234: [DOCS] ". Here is an example of a good commit message: http://gerrit.cloudera.org:8080/#/c/15877/9/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/15877/9/be/src/exprs/expr-test.cc@3892 PS9, Line 3892: nit: we don't usually have a space before parentheses here and below. It's probably worth running clang-format on your patches - it automates some of this minor stuff https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/15877/9/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java File fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java: http://gerrit.cloudera.org:8080/#/c/15877/9/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@33 PS9, Line 33: private Expr encapsulatedExpr_; Can you comment that this is initialized during analysis. http://gerrit.cloudera.org:8080/#/c/15877/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/15877/9/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3099 PS9, Line 3099: select distinct bool_col || int_col < smallint_col, "TEST" || string_col from functional.alltypes WHERE float_col = double_col || (string_col || 'test') = 'testtest'; Can you reformat this so that it's more readable (i.e. not all one one line). Same for the one below. -- To view, visit http://gerrit.cloudera.org:8080/15877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3f990d56ecb1e18d1b2737e8c5eab0d524edfaf Gerrit-Change-Number: 15877 Gerrit-PatchSet: 9 Gerrit-Owner: Martin Zink <martin.z...@protonmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Martin Zink <martin.z...@protonmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Jun 2020 00:58:29 +0000 Gerrit-HasComments: Yes
