Gabor Kaszab 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) Hey Tim, I'm fine with the change on the whole just have some very minor 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 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 successfully before introducing these here. 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. I'd find DCHECK too heavy here as they are already checked in the beginning of TimestampParser::Parse() 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 move the error handling in one function. I wonder how this could be more straightforward. Another name might be more suitable: IndicateTimestampParseFailure(). What do you think? -- 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 15:49:00 +0000 Gerrit-HasComments: Yes
