Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/13722 )
Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1 ...................................................................... Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-iso-sql-format-tokenizer.cc File be/src/runtime/datetime-iso-sql-format-tokenizer.cc: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@114 PS2, Line 114: ProcessNextToken Mention in a comment that this function is doing greedy-matching and finds the longest matching token. http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@122 PS2, Line 122: auto const auto http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@140 PS2, Line 140: auto const auto http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.h@67 PS2, Line 67: If it failed then nit: Otherwise http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@43 PS2, Line 43: nit: Double space. http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@43 PS2, Line 43: nit:double space. http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@130 PS2, Line 130: boost::posix_time::time_duration No need for a fully qualified name, 'time_duration' is available in this namespace. http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@150 PS2, Line 150: int day_offset = AdjustWithTimezone(t, dt_result.tz_offset); Before this change, timezone-adjustment was done only when dt_ctx.has_time_toks was set. Any reason you changed this behavior? Isn't this a breaking change? http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@231 PS2, Line 231: tok.len != 4 tok.len < 4 http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/timestamp-parse-util.cc@232 PS2, Line 232: int adjust_factor = std::pow(10, tok.len); : num_val %= adjust_factor; Isn't this a breaking change. "% 100" vs "% adjust_factor" ? http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/service/impala-server.cc@1119 PS2, Line 1119: query_ctx->__set_now_string(query_ctx->client_request.query_options.now_string); Is this query option used for testing only? Please add a comment to explain. http://gerrit.cloudera.org:8080/#/c/13722/2/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/13722/2/common/thrift/Exprs.thrift@144 PS2, Line 144: TCastExpr Consider renaming to TCastFormatExpr or something similar to emphasize that this is only used for cast expressions with a format clause. http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/main/cup/sql-parser.cup@2937 PS2, Line 2937: cast_format_expr Consider renaming this to 'cast_format_val' http://gerrit.cloudera.org:8080/#/c/13722/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/13722/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3088 PS2, Line 3088: public void TestCastFormatClauseFromTimestamp() throws AnalysisException { : String to_timestamp_cast = "cast('05-01-2017' as timestamp)"; : AnalysisError("select cast("+to_timestamp_cast+" as DATETIME FORMAT 'MM-dd-yyyy')", : "Unsupported data type: DATETIME"); : AnalysisError("select cast("+to_timestamp_cast+" as DATE FORMAT 'MM-dd-yyyy')", : "FORMAT clause is not applicable from TIMESTAMP to DATE"); : 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')"); : } Would it make sense to add similar tests for DATE instead of TIMESTAMP? http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3109 PS2, Line 3109: String cast_str = "CAST('05-01-2017' AS TIMESTAMP FORMAT 'MM-dd-yyyy')"; : SelectStmt select = (SelectStmt) AnalyzesOk("select " + cast_str); : Assert.assertEquals(cast_str, select.getResultExprs().get(0).toSqlImpl()); Please add similar test for DATE instead of TIMESTAMP. http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1458 PS2, Line 1458: ParsesOk("select cast('05-01-2017' as timestamp format 'MM-dd-yyyy')"); : ParserError("select cast(a + 5.0 as badtype) from t"); : ParserError("select cast(a + 5.0, string) from t"); : ParserError("select cast('05-01-2017' as timestamp format 12345)"); : ParserError("select cast('05-01-2017' as timestamp format )"); Please add similar tests for DATE. -- To view, visit http://gerrit.cloudera.org:8080/13722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19d8d097a45ae6f103b6cd1b2d81aad38dfd9e23 Gerrit-Change-Number: 13722 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Jul 2019 15:29:49 +0000 Gerrit-HasComments: Yes
