Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/15130 )
Change subject: IMPALA-9280: Fix parsing of timestamp with dash before TZH ...................................................................... Patch Set 2: (3 comments) Thanks for taking care of this! http://gerrit.cloudera.org:8080/#/c/15130/2/be/src/runtime/datetime-iso-sql-format-parser.cc File be/src/runtime/datetime-iso-sql-format-parser.cc: http://gerrit.cloudera.org:8080/#/c/15130/2/be/src/runtime/datetime-iso-sql-format-parser.cc@332 PS2, Line 332: && cur_pos_move_count > cur_tok_move_count && *(*current_pos) != '+' Counting the processed separator tokens doesn't seem the best approach for me here. Also I don't see why it is necessary to have more separators in the input than in the format to use the last minus sign as a part of TZH instead of taking it as a separator. Previously this if condition said "If a TZH token follows the separator sequence and the last separator in the sequence was a minus char then I'll use that minus char as the sign of the TZH". I'd extend this: "If a TZH token follows the separator sequence and the last separator in the sequence was a minus char and the TZH token doesn't start with a plus char then ..." Please make sure that in case the minus char is actually taken as sign of TZH instead of a separator then there is still at least one separator remaining in the input sequence to match the separator sequence in the token list. http://gerrit.cloudera.org:8080/#/c/15130/2/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/15130/2/be/src/runtime/timestamp-test.cc@1071 PS2, Line 1071: TEST(TimestampTest, IsoSqlWithTZ) { Almost all of these formatting tests are in tests/query_test/test_cast_with_format.py I think we should keep them together. Please consider moving this coverage. http://gerrit.cloudera.org:8080/#/c/15130/2/be/src/runtime/timestamp-test.cc@1078 PS2, Line 1078: HH:MI-TZH As far as I know we don't really support dateless timestamps even though Impala accepts them. Additionally I think we should remove the support of them from Impala at one point (AFAIK JDBC/ODBC connections throw parse error for dateless timestamps currently). So might be safer not to use them even in tests as once we drop the support we have to rewrite less code. -- To view, visit http://gerrit.cloudera.org:8080/15130 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I24c636e75fd380f6ebd091bcb38bb60a274f0e00 Gerrit-Change-Number: 15130 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 30 Jan 2020 09:00:02 +0000 Gerrit-HasComments: Yes
