Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/12267 )
Change subject: IMPALA-4018 Part1: Add FORMAT clause in CAST() ...................................................................... Patch Set 3: (8 comments) I took a quick look. Some minor issues; http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/CMakeLists.txt File be/src/exprs/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/CMakeLists.txt@33 PS3, Line 33: cast-expr.h Is adding header files necessary? http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/cast-functions-ir.cc@167 PS3, Line 167: string const string http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/cast-functions-ir.cc@283 PS3, Line 283: string const string http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/12267/3/be/src/exprs/scalar-expr.h@162 PS3, Line 162: string In headers use fully qualified names: std::string http://gerrit.cloudera.org:8080/#/c/12267/3/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/3/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@88 PS3, Line 88: Preconditions.checkNotNull(targetTypeDef); : Preconditions.checkNotNull(e); : isImplicit_ = false; : targetTypeDef_ = targetTypeDef; : castFormat_ = null; : children_.add(e); Can you call here 'this(targetTypeDef, e, null)' instead? http://gerrit.cloudera.org:8080/#/c/12267/3/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@228 PS3, Line 228: .addValue(super.debugString()) : .add("format", castFormat_) I think these two lines whould be switched http://gerrit.cloudera.org:8080/#/c/12267/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/12267/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2926 PS3, Line 2926: public void TestCastFormatClauseFromString() throws AnalysisException { : AnalysisError("select cast('05-01-2017' AS DATE FORMAT 'MM-dd-yyyy')", : "Unsupported data type: DATE"); : AnalysisError("select cast('05-01-2017' AS DATETIME FORMAT 'MM-dd-yyyy')", : "Unsupported data type: DATETIME"); : AnalysisError("select cast('05-01-2017' AS INT FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from STRING to INT"); : AnalysisError("select cast('05-01-2017' AS STRING FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from STRING to STRING"); : AnalysisError("select cast('05-01-2017' AS BOOLEAN FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from STRING to BOOLEAN"); : AnalysisError("select cast('05-01-2017' AS DOUBLE FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from STRING to DOUBLE"); : AnalysisError("select cast('05-01-2017' AS TIMESTAMP FORMAT '')", : "FORMAT clause can't be empty"); : AnalyzesOk("select cast('05-01-2017' AS TIMESTAMP FORMAT 'MM-dd-yyyy')"); : } : : @Test : public void TestCastFormatClauseFromTimestamp() throws AnalysisException { : String to_timestamp_cast = "cast('05-01-2017' as timestamp)"; : AnalysisError("select cast("+to_timestamp_cast+" as DATE FORMAT 'MM-dd-yyyy')", : "Unsupported data type: DATE"); : AnalysisError("select cast("+to_timestamp_cast+" as DATETIME FORMAT 'MM-dd-yyyy')", : "Unsupported data type: DATETIME"); : AnalysisError("select cast("+to_timestamp_cast+" AS INT FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from TIMESTAMP to INT"); : AnalysisError("select cast("+to_timestamp_cast+" AS BOOLEAN FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from TIMESTAMP to BOOLEAN"); : AnalysisError("select cast("+to_timestamp_cast+" AS DOUBLE FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from TIMESTAMP to DOUBLE"); : AnalysisError("select cast("+to_timestamp_cast+" AS STRING FORMAT '')", : "FORMAT clause can't be empty"); : AnalyzesOk("select cast("+to_timestamp_cast+" AS STRING FORMAT 'MM-dd-yyyy')"); : AnalyzesOk("select cast("+to_timestamp_cast+" AS VARCHAR FORMAT 'MM-dd-yyyy')"); : AnalyzesOk("select cast("+to_timestamp_cast+" AS CHAR(10) FORMAT 'MM-dd-yyyy')"); : } Maybe you should add tests for short format strings, e.g.: "yyyy-M-d" http://gerrit.cloudera.org:8080/#/c/12267/3/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/3/testdata/workloads/functional-query/queries/QueryTest/cast_format.test@62 PS3, Line 62: yyyy/dd/MM HH:mm:ss Again, add some tests using short format strings, e.g. yyyy-M-d -- 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: 3 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[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-Comment-Date: Thu, 31 Jan 2019 16:58:10 +0000 Gerrit-HasComments: Yes
