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 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5746 PS3, Line 5746: TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 00|00|00',\ > Thanks for the explanation! I guess your question is that the result seems to be wrong even though the format is find and everything is okey to run. Timezone can affect epoch timestamp. Impala interprets date and time with UTC timezone by default due to avoidance of undesired results by local timezone. - Regarding default timezone: https://github.com/apache/impala/blob/master/docs/topics/impala_timestamp.xml#L89 - Regarding data and time interpretation at unix_timestamp: https://github.com/apache/impala/blob/master/docs/topics/impala_datetime_functions.xml#L2463 - Test case for epoch: https://github.com/apache/impala/blob/master/be/src/exprs/expr-test.cc#L5617 Therefore, I think the result should be zero. http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@182 PS4, Line 182: Reset(fmt, fmt_len); > 1: This comment doesn't add any further info as it is basically the functi Thanks for your kind comment. 1,2: I agree. Actually I thought I should have to add the meaningless comment, but I didn't imagine more meaningful comment and I think the function name looks self-descriptive. 3. More detailed explanation is added to return description instead of the word "parse". 4. I agree. Reader expects a string literal due to "Get", so the revised function will return a pointer of string literal. The function is split into two parts: GetStringLiteralBetweenQuotes and SaveDateTimeToken. I think SaveDateTimeToken will be used generally when refactoring ParseFormatTokens or adding a new code. I fully agree an unit function should have a minimal purpose and responsibility. http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@183 PS4, Line 183: } > Again, this is simply writing the type of the parameter with spaces. Actually I don't want leave meaningless comments. I think the parameter names are self-explainable, but please leave a comment as a new reader if you think any additional information should be helpful. -- 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: 3 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: Wed, 06 Dec 2017 11:41:16 +0000 Gerrit-HasComments: Yes
