> On March 23, 2014, 5:48 a.m., Jihoon Son wrote: > > Hi, Alvin. > > Since I already give +1 on JIRA, I'm very sorry to say to suggest as > > follows. > > > > You proposed UnixTimeStampDatum to use in DateTimePartFromUnixTimeStamp. As > > you said, UnixTimeStampDaum is based on the Unix time. > > However, TimestampDatum which is the default type for timestamps is also > > based on the Unix time. So, in my opinion, TimestampDatum and > > UnixTimeStampDatum are fundamentally same, but provide different functions. > > For more detailed differences between them, UnixTimeStampDatum provides > > specific functions to approximate DateTime based on a given time unit > > (year, month, day, ...), while the functions of TimestampDatum extract a > > certain portion from DateTime (year, monthOfYear, DayOfWeek, ...). > > I think that the approximation functions of UnixTimeStampDatum are very > > useful, but may be used only in DateTimePartFromUnixTimeStamp. > > > > So, how about moving the approximation functions of UnixTimeStampDatum to a > > utility class such as TimestampUtil? > > I think that putting functions for a specific operation together will > > increase the readability and make users less confusing. > > > > Again, I truly apologize for changing my former opinion in JIRA.
Hi Jihoon, Please don't be sorry... :) I was wondering why it was accepted in the first place because you are absolutely right.I just followed the pattern the way other Date and Time Datums were implemented to be on the safer side and I myself was debating am I doing this right because TimestampDatum already exist but the behavior of the functions were so different I created the UnixTimeStampDatum like day in TimestampDatum returns the day (MON , TUE and so on) and the day in UnixTimeStampDatum shifts it to start of the day as per Google Big Query implementation. It is a better idea to move the function to TimestampUtil and remove UnixTimeStampDatum class.I will update the patch as per the suggestion. Thanks! Warm Regards, Alvin. - Alvin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19513/#review38249 ----------------------------------------------------------- On March 23, 2014, 5 a.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19513/ > ----------------------------------------------------------- > > (Updated March 23, 2014, 5 a.m.) > > > Review request for Tajo and Hyunsik Choi. > > > Bugs: TAJO-684 > https://issues.apache.org/jira/browse/TAJO-684 > > > Repository: tajo > > > Description > ------- > > Add functions about time > > > Diffs > ----- > > tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java > b882e84 > > Diff: https://reviews.apache.org/r/19513/diff/ > > > Testing > ------- > > > File Attachments > ---------------- > > TAJO-684-Patch > > https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch > > > Thanks, > > Alvin Henrick > >
