Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5316: Adds last_day() function ......................................................................
Patch Set 2: (7 comments) Thanks for the contribution, Vincent. http://gerrit.cloudera.org:8080/#/c/6991/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 279: void TestLastDayFunction() { can you check the results of all of these tests are the same as a reference database? Line 281: TestTimestampValue("last_day('2003-01-02')", can you add times to some of these? Line 282: TimestampValue::Parse("2003-01-31 00:00:00", 19)); for all of the below, use the Parse() overload that takes std::string, i.e. remove the str len. Line 316: TimestampValue::Parse("2100-02-28 00:00:00", 19)); I pointed out a few special cases to consider in the .cc file In addition: '1400-01-01 00:00:00' (first supported date) '9999-12-31 23:59:59' (last supported date) http://gerrit.cloudera.org:8080/#/c/6991/2/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: PS2, Line 562: end_of_month() 1) I wonder if you can get a TimestampValue without a Date or with a "special" date, eg. maybe if you create a TIMESTAMP with just a time. Please test last_day(cast("00:00:00" as timestamp)); 2) I think we have to be sure to check the case when timestamp.date() is at the end of the supported date range (year 12-31-9999 23:59:59), I suspect it should return a valid date but we should check. I can't think of any other corner cases. PS2, Line 562: ptime pdate(timestamp.date().end_of_month()); : TimestampValue tsv(pdate); I think you can avoid converting to ptime and just use the TimestampValue constructor that takes a gregorian date and a time_duration, e.g. tsv(timestamp.date().end_of_month(), time_duration(0,0,0,0)) http://gerrit.cloudera.org:8080/#/c/6991/2/be/src/exprs/timestamp-functions.h File be/src/exprs/timestamp-functions.h: Line 198: static TimestampVal LastDay(FunctionContext* context, const TimestampVal& ts); please add a comment. look to other fns in the file for examples. -- To view, visit http://gerrit.cloudera.org:8080/6991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I429c8734bddca3c37a2eedc211a16a4ffcb04370 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vincent Tran <vtt...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
