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: (8 comments) http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc@5724 PS1, Line 5724: TestValue("unix_timestamp('1970|01|01 00|00|00', 'yyyy|MM|dd HH|mm|ss')", TYPE_BIGINT, > I think we also need to add tests for parsing with format strings including Thanks for the information. A quoted text does not work In the first patch set due to missing implementation. Done. http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc@5746 PS1, Line 5746: TestError("from_unixtime(0, 'HHH\\'')"); > Can you add a test for an unmatched '\. Or if one of the above tests covers 5745 and 5746 are testing for unterminated quote http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.h@77 PS1, Line 77: /// The following features are not supported: > Can you update this comment to include an example of the feature you added? Done http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@160 PS1, Line 160: if ('\'' == *str) { > nit: *str == '\'' to match the convention below Done http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@162 PS1, Line 162: do { > Can you add a comment here explaining that we're matching a string literal, Done http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@164 PS1, Line 164: // Meet end of string > This comment isn't to informative. Maybe: Done http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@165 PS1, Line 165: if (str == str_end) { > Nit: 1 line: Done http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@168 PS1, Line 168: } while ('\'' != *str); > nit: *str != '\'' to match the convention below 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: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 09 Nov 2017 06:28:32 +0000 Gerrit-HasComments: Yes
