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 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15130/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15130/4//COMMIT_MSG@11 PS4, Line 11: if the negative sign is the only possible : separator character in the separator substring. I have the same comment here as in the parser: This doesn't cover the whole picture as we also don't include it if the TZH itself has a '+' char as a sign. http://gerrit.cloudera.org:8080/#/c/15130/4/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/4/be/src/runtime/datetime-iso-sql-format-parser.cc@296 PS4, Line 296: (*current_pos) nit: no need for the brackets. http://gerrit.cloudera.org:8080/#/c/15130/4/be/src/runtime/datetime-iso-sql-format-parser.cc@323 PS4, Line 323: // The last '-' of a separator sequence might be taken as a sign for timezone hour. : // UNLESS the last '-' is the only possible separator character in the separator : // substring (in which case it is not counted as the timezone hour's negative sign). I'd rephrase this comment a bit as it misses to mention that if the TZH starts with a '+' then we again don't take the last '-' of the separator sequence as the sign. Let's say something like this: "If the TZH token itself doesn't start with a sign and the separator sequence contains more than one separators then the last '-' of the separator sequence is taken as the sign of TZH". What do you think? -- 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: 4 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: Fri, 31 Jan 2020 07:19:45 +0000 Gerrit-HasComments: Yes
