Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9391 )

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


Patch Set 2:

(8 comments)

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.

http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/expr-test.cc@6589
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?


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/expr-test.cc@6600
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?


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/expr-test.cc@6615
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


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins-ir.cc@118
PS2, Line 118: int64_t ExtractNanosecond(const time_duration& time) {
can these functions live inside UdfBuiltins class?


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins-ir.cc@135
PS2, Line 135: NANOS_PER_MICRO * MICROS_PER_MILLI
Introduce something like NANOS_PER_MILLI?


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins-ir.cc@140
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 
well.


http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins.cc
File be/src/exprs/udf-builtins.cc:

http://gerrit.cloudera.org:8080/#/c/9391/2/be/src/exprs/udf-builtins.cc@52
PS2, Line 52:   // Below units are only used by DateTrunc
This comment is not valid anymore, right?


http://gerrit.cloudera.org:8080/#/c/9391/2/common/thrift/Exprs.thrift
File common/thrift/Exprs.thrift:

http://gerrit.cloudera.org:8080/#/c/9391/2/common/thrift/Exprs.thrift@98
PS2, Line 98:   EPOCH,
Do you need that trailing comma?



--
To view, visit http://gerrit.cloudera.org:8080/9391
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <jinc...@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 16:32:16 +0000
Gerrit-HasComments: Yes

Reply via email to