Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/12267 )
Change subject: IMPALA-4018 Part1: Add FORMAT clause in CAST() ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.h File be/src/exprs/cast-expr.h: http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.h@43 PS5, Line 43: // This is empty in case of FunctionContext::THREAD_LOCAL. > I see, I forgot about the error path. Done http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java File fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java: http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java@93 PS6, Line 93: return new CastExpr(targetType, this); > Rather than changing calls everywhere, would recommend leaving the two-arg That sounds reasonable. Thanks for spotting. Done. http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/CastExpr.java File fe/src/main/java/org/apache/impala/analysis/CastExpr.java: http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@92 PS6, Line 92: public CastExpr(TypeDef targetTypeDef, Expr e) { > I suspect only one of these is called from the parser. That one can take th The implicit cast constructor used internally could also receive a format in case the cast target type is char[]. In that case the input is converted to string as an intermediate step and then to char[] as a final step. Check line #271 in this file. http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/Expr.java@1362 PS6, Line 1362: return new CastExpr(targetType, this); > See prior comment. Done http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java: http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@478 PS6, Line 478: return new CastExpr(targetType, this); > Again. Done http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java: http://gerrit.cloudera.org:8080/#/c/12267/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@157 PS6, Line 157: return new CastExpr(targetType, this); > Again. Done http://gerrit.cloudera.org:8080/#/c/12267/6/testdata/workloads/functional-query/queries/QueryTest/cast_format.test File testdata/workloads/functional-query/queries/QueryTest/cast_format.test: http://gerrit.cloudera.org:8080/#/c/12267/6/testdata/workloads/functional-query/queries/QueryTest/cast_format.test@4 PS6, Line 4: ==== > The tests here are great. Only suggestion is that we have to ensure we test That would be nice, however please note the following: - This is just a pattern handling re-use of existing functionality that already has coverage. - As discussed on the Jira, with CAST(FORMAT) I won't follow the existing pattern matching. Instead I'll implement the ISO SQL:2016 support so testing the current matching here doesn't make sense as I'll rewrite it anyway. Also please note, that this patch won't make it to the repo alone, I have to extend it to use the new datetime patterns and then send them as 1 patch together. http://gerrit.cloudera.org:8080/#/c/12267/5/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/12267/5/tests/query_test/test_cast_with_format.py@37 PS5, Line 37: def test_cast_format_without_row(self, vector): > I saw this pattern in several tests: Aaaaww, indeed. Thx! -- To view, visit http://gerrit.cloudera.org:8080/12267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia514aaa9e8f5487d396587d5ed24c7348a492697 Gerrit-Change-Number: 12267 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Mon, 18 Feb 2019 09:53:45 +0000 Gerrit-HasComments: Yes
