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 7: (5 comments) Thanks for getting back to this review! :) One general comment: Try to avoid sending code rebase and new changes in the same patch set as it's makes reviewing the changes between the current and the previous patch set more difficult. http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@189 PS7, Line 189: // This function returns three kinds of values via the output parameters. This sentence is not needed in my opinion. http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@190 PS7, Line 190: // str is to point to the opening quote when the function is called. Once the function str is assumed to point to... This seems more natural to me. What do you think? http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@192 PS7, Line 192: // position_of_string_literal is start position of the string literal. some separator between the param name and it's description would be great. e.g. check the function comments below. http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.cc@161 PS7, Line 161: const char*& str, int& position_of_string_literal, int& length_of_string_literal, One thing I learned recently, that in Impala the out parameters are preferred as a pointer instead of references. Tim, can you confirm? http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.cc@203 PS7, Line 203: ++str; It might worth a comment here that incrementing this would result in the str pointing to the first char after the closing quote. (I know it's mentioned in the comment for GetStringLiteralBetweenQuotes but it might increase readability repeating here.) -- 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: 7 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: Fri, 26 Jan 2018 14:22:28 +0000 Gerrit-HasComments: Yes
