Riza Suminto 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: (4 comments) Thank you Gabor for your explanation. 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@297 PS2, Line 297: int cur_pos_move_count = 0; > I think the algorithm used here could maybe be simplified. I don't want to Ack 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 What I understand from your explanation, the number of separator chars in the time string does not need to be as much as separator chars in the pattern. What matter is there should be at least one separator char in the time string. Therefore, given a pattern "HH:MI-TZH": a) "08:00-01" is equal to "08:00 +01" b) "08:00-+01" is equal to "08:00 +01" c) "08:00--01" is equal to "08:00 -01" d) "08:00---01" is equal to "08:00 -01" And all above is also true for pattern "HH:MI--TZH", is that correct? 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 Ack 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 Im Will follow example from tests/query_test/test_cast_with_format.py -- 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 <riza.sumi...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Jan 2020 17:52:10 +0000 Gerrit-HasComments: Yes
