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: (8 comments) Thanks for the potential problem for double quotes. 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: You found a potential problem in my change. I didn't consider a double quote. Let me provide a solution with including the case in the next patch set. 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 > Also mention please the side-effect of advancing the str param to the closi Sorry, I could not catch what you meant. Would you please explain it with an example? 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. I think it should be passed to this function because the function is used more in general. There are different token types. When we refactor several use cases, the function will be used. e.g. https://github.com/apache/impala/blob/master/be/src/runtime/timestamp-parse-util.cc#L165 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 eno As I mentioned in the comment of line#151, the function will be used broadly, so the essential checks are required. The function should be self-contained. 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! I think SaveDateTimeToken is not suitable to calculate the place of the position_of_string_literal. As I mentioned, the function will be used in general. The formula of the calculation is fit to here, so I don't think the formula can cover all the cases because there are a few different calculation logics. e.g. https://github.com/apache/impala/blob/master/be/src/runtime/timestamp-parse-util.cc#L169 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 o Not acceptable due to the reason above. 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 appro Okay, let me reflect this on the next patch set. 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 f Not acceptable due to the reason above. -- 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: Sun, 10 Dec 2017 12:00:49 +0000 Gerrit-HasComments: Yes
