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: (7 comments) Thanks, for answering/incorporating my comments! http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/exprs/expr-test.cc@5980 PS6, Line 5980: TestStringValue("from_unixtime(1492677561, 'HH\\'H\\'')", "08H"); I checked in Hive and it kind of handles nested quotes like: select from_unixtime(1492677561, "HH'H\"H\"' mm'm' ss's'"); 01H"H" 39m 21s Would it be possible to add a testcase to cover this? 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@151 PS6, Line 151: const DateTimeFormatTokenType token_type, const int position_of_token, SEPARATOR as token_type is not necessary as a param. dt_ctx, token_begin, token_length are sufficient as parameters as position_of_token could be calculated inside the function based on these, right? http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@153 PS6, Line 153: DCHECK(token_begin != nullptr); 2 of these DCHECKs are already done in the other function. I think it's enough to do the only once, e.g. here. 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) { Thanks for making these adjustments! Few more comments: I think this function should provide a pointer to the start of the string literal, and the length of the str (and as a side effect advancing the str pointer to the closing quote, that fact should be documented in the header file). SaveDateTimeToken seems a better place to calculate the position_of_string_literal. What do you think? http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@163 PS6, Line 163: const char* str_end = str_begin + dt_ctx->fmt_len; Can't we pass the str_end pointer to this function? Then we could get rid of dt_ctx param (I think str_begin is not needed in this function). http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@177 PS6, Line 177: ++str; At the end of the excution of this function I feel that it's a better approach to have the str pointing to the finishing quote. Before making a 'continue' it can be increased to the next char. My reasoning would be, that a function to get a string literal between quotes shouldn't be aware whether there are more characters after the finishing quote or not. It should only care about the chars between the quotes. http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@199 PS6, Line 199: SaveDateTimeToken(dt_ctx, SEPARATOR, position_of_string_literal, No need to pass the separator as a parameter, it is accessible inside the function too. -- 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: Thu, 07 Dec 2017 09:42:07 +0000 Gerrit-HasComments: Yes
