Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8508 )
Change subject: IMPALA-5237: Support a quoted string in date/time format ...................................................................... Patch Set 6: (6 comments) Thanks for your answers. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h@190 PS6, Line 190: // nullptr if input does not have unclosed quote > Before you call this function the str parameter points to the opening quote I see. Your point is that the pointer can be changeable in the function. What do you think about if I will add a comment like this? Please feel free to modify the comment or suggest one. The pointer of the str can be changeable because of advancing the str parameter to the closing quote. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@161 PS6, Line 161: const char*& str, int& position_of_string_literal, int& length_of_string_literal) { > Sure, thx for the explanation. I'm not sure we should prepare a function fo Thanks for your understanding. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@174 PS6, Line 174: DCHECK(length_of_string_literal >= 0); > Still I feel that making a CHECK on the same thing twice is an overkill. We Right, it should be a redundant. Let me remove this on the next patch set. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@176 PS6, Line 176: DCHECK(position_of_string_literal >= 0); Duplicate. Let me remove this on the next patch set. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@178 PS6, Line 178: return quoted_string_begin + 1; > In fact you have 3 different values returned from this function. 2 of them Do you think the function name(i.e. Get...) is still valid even though return value will move to an output parameter? I mean logically the function returns three kinds of outputs, but I guess a reader may think the function name looks weird because it should be void function. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@198 PS6, Line 198: if (UNLIKELY(!string_literal_begin)) return false; > Could you explain me why we have DCHECKs for position_of_string_literal, an As I mentioned at line#174 and line#176, they are not necessary. The checks at SaveDateTimeToken are enough. Let me remove them. -- To view, visit http://gerrit.cloudera.org:8080/8508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178 Gerrit-Change-Number: 8508 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Attila Jeges <atti...@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: Mon, 11 Dec 2017 10:45:27 +0000 Gerrit-HasComments: Yes
