Tim Armstrong 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 8: (9 comments) 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: TEST_F(ExprTest, QuotedStringForDateTimeFormat) { I found a case where your implementation is inconsistent with hive. It looks like hive treats '' inside quotes as an escape sequence for '. hive> select from_unixtime(1492677561, "HH'foo\'\'bar\'"); OK 01foo'bar [localhost:21000] > select from_unixtime(1492677561, "HH'foo\'\'bar\'"); Query: select from_unixtime(1492677561, "HH'foo\'\'bar\'") Query submitted at: 2018-02-07 16:58:04 (Coordinator: http://tarmstrong-box:25000) Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=aa4422fefbd02bcc:1f7de5ac00000000 +-----------------------------------------------+ | from_unixtime(1492677561, 'hh\'foo\'\'bar\'') | +-----------------------------------------------+ | 08foobar | +-----------------------------------------------+ 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: /// Finding the closing quote and getting the string between the quotes. Can you comment that there is no way to escape single quotes? http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h@188 PS8, Line 188: // str -- start pointer of the opening quote when the function is called. Once the nit: use /// for function comments. 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: SaveDateTimeToken Maybe DateTimeFormatContext::AppendToken() http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@150 PS8, Line 150: SaveDateTimeToken I think this should be a method of DateTimeFormatContext instead of TimestampParser. http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@155 PS8, Line 155: DCHECK Use DCHECK_GE macro to get better info on failure. http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@156 PS8, Line 156: push_back You can use emplace_back(token_type, position_of_token, ...) instead. http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@213 PS8, Line 213: dt_ctx->toks.push_back(DateTimeFormatToken(TZ_OFFSET, str - str_begin, Can you convert these other locations in this function to use your new helper function? We should be consistent... http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@518 PS8, Line 518: DCHECK((tok.pos + shift_len + tok.len) <= str_len); Use DCHECK_LE instead. -- 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: 8 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: Thu, 08 Feb 2018 00:59:51 +0000 Gerrit-HasComments: Yes
