Gabor Kaszab has posted comments on this change. ( )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Patch Set 2:


Thanks for taking care of this! I made a run through and in general the change 
seems great, I just have a few minor comments/questions.
File be/src/exprs/
PS2, Line 6589:   TestValue("extract(YEAR from cast('2006-05-12 
18:27:28.123456789' as timestamp))",
As I see you replaced all "12345" fractional seconds with 123456789. Would you 
leave few occurrences with the old, short one just in case?
PS2, Line 6600:             TYPE_BIGINT, 5);
Just for curiosity: Do you think it should be handled that in some cultures the 
weeks start with Monday and in other with Sunday?
PS2, Line 6615:   TestValue("extract(MICROSECOND from cast('2006-05-12 
18:27:28.123456789' as timestamp))",
Also try this on '2006-05-12 18:27:28' or '2006-05-12 18:27:28.12' please
File be/src/exprs/
PS2, Line 118: int64_t ExtractNanosecond(const time_duration& time) {
can these functions live inside UdfBuiltins class?
Introduce something like NANOS_PER_MILLI?
PS2, Line 140: TExtractField::type StrToExtractField(FunctionContext* ctx, 
const StringVal& unit_str) {
As Tim mentioned in the Jira, decade, century and millenium would be great as 
File be/src/exprs/
PS2, Line 52:   // Below units are only used by DateTrunc
This comment is not valid anymore, right?
File common/thrift/Exprs.thrift:
PS2, Line 98:   EPOCH,
Do you need that trailing comma?

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
Gerrit-Change-Number: 9391
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Kim Jin Chul <>
Gerrit-Comment-Date: Wed, 07 Mar 2018 16:32:16 +0000
Gerrit-HasComments: Yes

Reply via email to