Youwei Wang 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. Done http://gerrit.cloudera.org:8080/#/c/1943/2/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: Line 568: std::string cxx_string = std::string( > use a variable name that better describes the content, maybe weekday_str. Done Line 571: :: > do you need the ::? Done Line 573: day_index > day_idx, initialize to 0; Done Line 591: iDeltaDays > should follow naming convention, e.g. delta_days or num_delta_days. Done Line 591: int32_t > use either int or int32_t consistently throughout the function. the rest of Done Line 592: iDeltaDays = iDeltaDays <= 0 ? iDeltaDays + 7 : iDeltaDays; > add another DCHECK after this statement to make sure its in 1..7 Done http://gerrit.cloudera.org:8080/#/c/1943/2/be/src/exprs/timestamp-functions.h File be/src/exprs/timestamp-functions.h: Line 148: /// Returns the date of the weekday that follows a particular date. > Return (drop the s) Done Line 149: /// The weekday argument is a string literal indicating the day of the week. > put in single quotes Done Line 155: /// select next_day('2013-12-25','Saturday') returns '2013-12-28' > explain how different string formats are affected in the comment. Done Line 156: static StringVal NextDay(FunctionContext* context, const TimestampVal& date, > Why not return a timestamp val here? Oracle seems to do that: "The return v Done -- 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
