Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10349 )
Change subject: IMPALA-6995: avoid DCHECK in TimestampParse::Parse() ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/10349/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10349/2//COMMIT_MSG@7 PS2, Line 7: avoid > nit: Avoid Different people just capitalise this differently if you look through the commit history http://gerrit.cloudera.org:8080/#/c/10349/2/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/10349/2/be/src/runtime/timestamp-parse-util.cc@88 PS2, Line 88: const int TimestampParser::DEFAULT_DATE_FMT_LEN; > For my understanding, why are these needed? As far as I see these were used They should have been here before but previous the generated code didn't need to reference the address of these variables. I think the use of min() changed the generated code enough that it caused problems (there was a linker error). http://gerrit.cloudera.org:8080/#/c/10349/2/be/src/runtime/timestamp-parse-util.cc@372 PS2, Line 372: // when parsing fails. > Please mention in the comment that 'd' and 't' is expected not to be NULL. Done http://gerrit.cloudera.org:8080/#/c/10349/2/be/src/runtime/timestamp-parse-util.cc@373 PS2, Line 373: static bool TimestampParseFailed(date* d, time_duration* t) { > I find the return value of this function a bit weird. I see the intent to m Done -- To view, visit http://gerrit.cloudera.org:8080/10349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02a18ffd8893fe74f5830144300f745ce31477b1 Gerrit-Change-Number: 10349 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 15 May 2018 16:08:41 +0000 Gerrit-HasComments: Yes
