----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19513/#review38249 -----------------------------------------------------------
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. - Jihoon Son On March 23, 2014, 2 p.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19513/ > ----------------------------------------------------------- > > (Updated March 23, 2014, 2 p.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 > >
