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) ...................................................................... Patch Set 5: (9 comments) This will be great to have. The overall approach of instantiating a wrapper and then replacing during rewrites makes sense to me. http://gerrit.cloudera.org:8080/#/c/15877/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15877/5//COMMIT_MSG@11 PS5, Line 11: -|| (KW_LOGICAL_OR) creates CompoundVerticalBarExpr which expects two BOOLEAN operands or two STRING operands nit: formatting of commit message is weird. http://gerrit.cloudera.org:8080/#/c/15877/5//COMMIT_MSG@15 PS5, Line 15: I think this patch needs more tests -it needs tests that it returns the correct results in more cases - for different data types, etc, in expr-test.cc. It'd also be good to add at least one end-to-end test in test_exprs.py (probably it would live in exprs.test) to make sure it works end-to-end (we have had cases before where exprs work fine in all the tests, but something wasn't set up right for them to work when running in the context of a real query). http://gerrit.cloudera.org:8080/#/c/15877/5//COMMIT_MSG@17 PS5, Line 17: Can you also add some cases for ToSqlTest.java to make sure that that works as expected. http://gerrit.cloudera.org:8080/#/c/15877/5/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/5/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@33 PS5, Line 33: logicalOR nit: members have trailing _ in our Java coding style http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@34 PS5, Line 34: private FunctionCallExpr concatFunctionCall; Why not just have a single member of type Expr. I think that would reduce the amount of code below significantly - you wouldn't need all the if () statements. http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@40 PS5, Line 40: children_.add(e1); nit: indentation is off http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/analysis/CompoundVerticalBarExpr.java@85 PS5, Line 85: nit: space after getType http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java File fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java: http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java@31 PS5, Line 31: nit: inconsistent blank line http://gerrit.cloudera.org:8080/#/c/15877/5/fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java@41 PS5, Line 41: nit: whitespace before () -- 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: 5 Gerrit-Owner: Martin Zink <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2020 19:35:17 +0000 Gerrit-HasComments: Yes
