impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kim Jin Chul (Code Review)" <>
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. ( )

Change subject: IMPALA-5237: Support a quoted string in date/time format

Patch Set 3:

File be/src/exprs/
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:
- Regarding data and time interpretation at unix_timestamp:

- Test case for epoch:

Therefore, I think the result should be zero.
File be/src/runtime/timestamp-parse-util.h:
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.
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Kim Jin Chul <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Wed, 06 Dec 2017 11:41:16 +0000
Gerrit-HasComments: Yes

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