impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kim Jin Chul (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format
Date Wed, 06 Dec 2017 11:41:16 GMT
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 <jinchul@gmail.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <jinchul@gmail.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 11:41:16 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message