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 2: (8 comments) Added a few comments. Verified that CAST(expression AS type FORMAT string) is in the latest SQL standards. Is the functionality here fully compatible with the existing from_timestamp() function? A quick check of the manual suggests that Impala does not support the Postgres to_char(timestamp, format) function. Should we add that to be compatible with Postres and to offer functions for conversions to/from timestamps? (http://www.postgresqltutorial.com/postgresql-date/) http://gerrit.cloudera.org:8080/#/c/12267/2/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/12267/2/common/thrift/Exprs.thrift@143 PS2, Line 143: } Again, should format be an argument to the cast function for DateTime/string and string/datetime? http://gerrit.cloudera.org:8080/#/c/12267/2/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/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@50 PS2, Line 50: private String castFormat_; final http://gerrit.cloudera.org:8080/#/c/12267/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@220 PS2, Line 220: } If cast is implemented as a function call (which it seems to be), should the format be represented as another argument to that function rather than adding a new cast_expr field to all expressions (in the base TExprNode)? http://gerrit.cloudera.org:8080/#/c/12267/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@256 PS2, Line 256: if (null != castFormat_ && !castFormat_.isEmpty() && Here we are saying that CAST(col AS DATETIME FORMAT '') is the same as CAST(col AS DATETIME)? A broader question: should the format itself be validated so we catch errors here rather than in the BE? http://gerrit.cloudera.org:8080/#/c/12267/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@272 PS2, Line 272: tostring.castFormat_ = castFormat_; Pass into constructor so that castFormat_ can be final? http://gerrit.cloudera.org:8080/#/c/12267/2/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/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2957 PS2, Line 2957: AnalyzesOk("select cast("+to_timestamp_cast+" AS CHAR(10) FORMAT 'MM-dd-yyyy')"); Checks for an empty or invalid format string? http://gerrit.cloudera.org:8080/#/c/12267/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2964 PS2, Line 2964: Assert.assertEquals(select.getResultExprs().get(0).toSqlImpl(), cast_str); Order in JUint assertions is expected, actual. It is reversed in the line above. http://gerrit.cloudera.org:8080/#/c/12267/2/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/2/testdata/workloads/functional-query/queries/QueryTest/cast_format.test@20 PS2, Line 20: ---- QUERY Add error cases? Bad format, etc? Formats seem to be checked at runtime so need to be tested in an EE test. -- 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: 2 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 30 Jan 2019 18:41:40 +0000 Gerrit-HasComments: Yes
