Gabor Kaszab 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: (5 comments) Thanks for the explanation! 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 > Sorry, I could not catch what you meant. Would you please explain it with a Before you call this function the str parameter points to the opening quote. When the function returns it points to the closing quote. I think you should mention this side effect (of moving the str pointer to the closing quote) in the function's comment. 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) { > I think SaveDateTimeToken is not suitable to calculate the place of the pos Sure, thx for the explanation. I'm not sure we should prepare a function for general use even though currently we want to use to only for this purpose. But I accept your point. 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 can be sure, that SaveDateTimeToken is called right after this function so I think that making the DCHECKs there is sufficient. 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 are output parameters and one is a return value. I think in this case making all 3 of them as an output parameter would make sense. 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, and length_of_string_literal while we make an if-check on string_literal_begin? I think that could also be a DCHECK and can be done inside SaveSateTimeToken. -- 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 <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 11 Dec 2017 10:19:39 +0000 Gerrit-HasComments: Yes
