Martin Zink has posted comments on this change. ( http://gerrit.cloudera.org:8080/15877 )
Change subject: IMPALA-452: Add support for string concatenation operator using || ...................................................................... Patch Set 12: (11 comments) Hell... I thought when I publish the next patchset gerrit will finalize the draft commands. Sorry about that. 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 || > commit message is still weirdly formatted. https://cwiki.apache.org/conflue Done http://gerrit.cloudera.org:8080/#/c/15877/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15877/10//COMMIT_MSG@22 PS10, Line 22: > I compared the behaviour of this operator to postgres and hive, and it look Yeah that is a good idea, I will also check out the different behaviors of different SQL parsers regarding the implicit casts. Follow-up Jira seems a good way, I would love to work on that. http://gerrit.cloudera.org:8080/#/c/15877/10//COMMIT_MSG@22 PS10, Line 22: > We should also add some more tests to AnalyzeExprsTest.java that test both I've added a bunch of positive and negative test cases to AnalyzeExprsTest 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 Yeah old habits die hard, didn't know about the clang tidy this should not be an issue anymore thanks :) http://gerrit.cloudera.org:8080/#/c/15877/10/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/15877/10/be/src/exprs/expr-test.cc@3893 PS10, Line 3893: TestStringValue("cast('foo ' AS CHAR(5)) || '3'", "foo 3"); > Can you add some tests for string concatenation with NULL STRING arguments? Originally OR accepted NULL so I've kept that argument type solely for the logical OR version. (changing the least amount compared to previous behaviour). I've added some test cases here and to AnalyzeStmtsTest.java as well to reflect this behavior. http://gerrit.cloudera.org:8080/#/c/15877/11/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/15877/11/be/src/exprs/expr-test.cc@3893 PS11, Line 3893: TestStringValue("cast('foo ' AS CHAR(5)) || '3'", "foo 3"); > I think we also want a positive test case, e.g. Done http://gerrit.cloudera.org:8080/#/c/15877/11/be/src/exprs/expr-test.cc@3896 PS11, Line 3896: TestIsNull("cast(NULL AS STRING) || 'abc'", TYPE_STRING); > Also the reverse, i.e. 'abc' || NULL. Since the condition in the planner ch Done 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: > Can you comment that this is initialized during analysis. Done http://gerrit.cloudera.org:8080/#/c/15877/10/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/10/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@80 PS10, Line 80: encapsulatedExpr_.analyze(analyzer); > It's weird that we get different errors for, say string || int, vs int || s Agreed, I also reworked the error message to reflect that both CHAR, and VARCHAR can be parsed with STRINGs (reflecting how isStringType() works) http://gerrit.cloudera.org:8080/#/c/15877/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/15877/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@849 PS10, Line 849: RewritesOk("FALSE || id = 0", extractCompoundVerticalBarExprRule, "FALSE OR id = 0"); > Can we add a few more test cases for mixed types. E.g. a couple of cases of Sure, I've added those test cases to AnalyzeStmtsTest.java (they would not fit here, because they fail during analysis not rewrite phase) 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, > Can you reformat this so that it's more readable (i.e. not all one one line Done -- 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: 12 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: Mon, 22 Jun 2020 20:14:30 +0000 Gerrit-HasComments: Yes
