Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12267 )
Change subject: IMPALA-4018 Part1: Add FORMAT clause in CAST() ...................................................................... Patch Set 6: (6 comments) Great progress. Mostly nits, and one question about tests. 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, null); Rather than changing calls everywhere, would recommend leaving the two-arg constructor and adding a three-hard version. The two-arg version can call the three-arg version with null in the last position. 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, String format) { I suspect only one of these is called from the parser. That one can take the third arg. The other one, called only internally, will never pass a format. 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, null); See prior comment. 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, null); Again. 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, null); Again. 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 each and every format in the two-and four-digit versions. Would be great of we could do that in Java, but it requires the BE, so have to do it here. -- 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: 6 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: Thu, 14 Feb 2019 03:10:32 +0000 Gerrit-HasComments: Yes
