Those parts have poor documentation. I agree with your investigation. I also could find many misuse of timezone in many parts. We should make them clear and fix them in this chance.
I just got off the plane, and I'm still on the road. So, I'll give more comments tomorrow. - hyunsik On Thu, Nov 20, 2014 at 5:24 PM, Jaewoong Jung <[email protected]> wrote: > Wow, this seemingly trivial issue has surprisingly many problems involved. > > The most critical one, though, is TimeMeta class. Presumably because > it is poorly documented, it is being used for two different purposes > in Tajo code base. Some code treats it as a date time representation, > which I believe is the original intention, but some treat it as the > human-readable equivalent of TimeDatum by completely ignoring > date-related fields. > > For example, DateTimeUtil.date2j(long julianDate, TimeMeta tm), which > converts a julian timestamp to a TimeMeta value, doesn't touch > dayOfMonth, monthOfYear, or years values and just puts all values for > hour and above units in hours field. > > There are other minor problems like incorrect comments and absent > default values, but the most critical one is misuse of TimeMeta. > > I'll try to break up my patches so that each has a clear and > easy-to-understand goal. Sending this heads-up to let you know > there'll be more issues filed and patches sent than you might expect. > > On Wed, Nov 19, 2014 at 10:56 PM, Jaewoong Jung <[email protected]> wrote: > > Yeah, after some more research, I found that TImeDatum is a somewhat > > ambiguous data type. > > > > Its original purpose is to represent a time of a day, i.e. hh:mm:ss > > part of an instant. So, it could be viewed as instant data though some > > may argue it's incomplete to fully represent an instant. Anyway, given > > that TimestampDatum has limitation in terms of the time range it can > > represent, and given that Tajo doesn't have DateTime data type, > > clients should be allowed to use it with a timezone. > > > > I'll change the direction and try to address the issue by fixing the > > underflow error. > > > > Thanks for the input. :) > > > > On Wed, Nov 19, 2014 at 10:24 PM, Jong-young Park <[email protected]> > wrote: > >> Hi, Jaewoong. > >> > >> To express time period value, IntervalDatum is existing as I know. > >> > >> So I think it is right that TimeDatum is for some time point. > >> > >> And TimestampDatum seems it is doing both roles of DateDatum and > TimeDatum. > >> > >> Regards, > >> Jongyoung. > >> > >> On Wed Nov 19 2014 at 오후 5:23:40 Jaewoong Jung <[email protected]> > wrote: > >> > >>> It turns out, TAJO-1191 is slightly more complicated than I thought. > >>> (https://issues.apache.org/jira/browse/TAJO-1191) > >>> > >>> Basically, it's about whether TimeDatum may have a timezone tied with > >>> it. I **believe** TimeDatum is originally designed to hold a time > >>> period value, not an instant. (TimestampDatum seems to be the > >>> canonical container for instants.) So, it doesn't make sense to apply > >>> any timezones to TimeDatum values, but it's being done in a few > >>> places. And, that's why the test is failing on my machines. > >>> > >>> I'm going to try to fix it by removing all timezone-related references > >>> around the class, but I want to check my assumption with you before I > >>> proceed. > >>> > >>> What do you think about it? > >>> >
