[Impala-CR](cdh5-trunk) IMPALA-2459: Implement next_day date/time UDF

["Lars Volker (Code Review)" <ge...@cloudera.org>]

Lars Volker has posted comments on this change.

Change subject: IMPALA-2459: Implement next_day date/time UDF
......................................................................


Patch Set 2:

(11 comments)

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

Line 4235:   TestStringValue("next_day('2013-12-25','Saturday')", "2013-12-28");
Also test other input date formats and invalid inputs.


http://gerrit.cloudera.org:8080/#/c/1943/2/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:

Line 568: cxx_string
use a variable name that better describes the content, maybe weekday_str.


Line 571: ::
do you need the ::?


Line 573: day_index
day_idx, initialize to 0;


Line 591: int32_t
use either int or int32_t consistently throughout the function. the rest of the file seems to favor int.


Line 591: iDeltaDays
should follow naming convention, e.g. delta_days or num_delta_days.


Line 592: iDeltaDays
add another DCHECK after this statement to make sure its in 1..7


http://gerrit.cloudera.org:8080/#/c/1943/2/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:

Line 148: Returns
Return (drop the s)


Line 149: weekday
put in single quotes


Line 155:   /// select next_day('2013-12-25','Saturday') returns '2013-12-28'
explain how different string formats are affected in the comment.


Line 156: StringVal
Why not return a timestamp val here? Oracle seems to do that: "The return value has the same hours, minutes, and seconds component as the argument date." (from http://psoug.org/definition/NEXT_DAY.htm)
On the other hand Netezza returns a string in the same format as the input. Have you discussed which behavior we want to implement with someone from the team?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2721d236c096639a9e7d2df8a45ca888c6b3e83e
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang <42...@qq.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <42...@qq.com>
Gerrit-HasComments: Yes