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 1: (10 comments) Thanks for the comments. I've applied the update. http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/exprs/expr-test.cc@6622 PS8, Line 6622: int expected_var_begin, const map<int, set<int>>& expected_offsets) { > I found a case where your implementation is inconsistent with hive. It look Done. Thanks for finding the corner case. The case is added. http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h@187 PS8, Line 187: /// len -- length of the string to parse (must be > 0) > This might change if you fix the inconsistency I mentioned. As you mentioned in the corner case, I've fixed the inconsistency on the last patch set. See https://gerrit.cloudera.org/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc (#line 177) http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h@188 PS8, Line 188: /// dt_ctx -- date/time format context (must contain valid tokens) > nit: use /// for function comments. Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@150 PS8, Line 150: ParseFormatTokens > I think this should be a method of DateTimeFormatContext instead of Timesta Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@150 PS8, Line 150: ParseFormatTokens > Maybe DateTimeFormatContext::AppendToken() Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@155 PS8, Line 155: const > Use DCHECK_GE macro to get better info on failure. Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@156 PS8, Line 156: tr_end = > You can use emplace_back(token_type, position_of_token, ...) instead. Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@213 PS8, Line 213: while (curr_tok_chr < str_end) { > Can you convert these other locations in this function to use your new help Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@518 PS8, Line 518: boost::unordered_map<StringValue, int>::const_iterator iter = > Use DCHECK_LE instead. Done http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@519 PS8, Line 519: TH_INDE > I think using memcmp() would be better, since strncmp() is meant for null-t Done -- 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: 1 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, 19 Feb 2018 04:38:29 +0000 Gerrit-HasComments: Yes
