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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
