Tianyi Wang has posted comments on this change. Change subject: IMPALA-5867: Fix bugs parsing 2-digit year ......................................................................
Patch Set 1: (3 comments) Some explanation on Hive. Will address other comments. http://gerrit.cloudera.org:8080/#/c/7910/1/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: Line 68: century_break_ptime = boost::posix_time::ptime( > Mention that we set it to March 1 for consistency with Hive? This is not meant to be consistent with Hive but to not crash. See comment on L352. Line 352: if (dt_result.month == 2 && dt_result.day == 29 && > I don't fully understand this condition. Couldn't this advance 100 years ev There isn't 1900-02-29 and boost::gregorian::date would throw an exception if you try to construct it. So without this if block any 00-02-29 will be invalid date. There are many ways to workaround it and I'm not sure which one looks best. By the way, this is not what java.util.Calendar does. In Java if now() is 1980-02-29 18:00:00 the break time will be set to 1900-02-28 18:00:00 and any 02-29 will be parsed as 2000-02-29. But if now() is 1980-03-01 18:00:00, 02-29 17:00:00 will be 2000-02-29 but 02-29 19:00:00 will be 1900-03-01. It seems 1900-02-29 exists in Java, is comparable to a time in 1900-03-01, and will be 2000-02-29 if you add 100 years to it, which is nothing like boost. So trying to be exactly same as Hive would really add some irregular logic here. Line 370: if (d->year() < 1400 || d->year() > 9999) { > I'm not sure if this check is necessary now - there was an off-by-one bug i Boost still won't throw an exception when you add -1 days to 1400-01-01. But if you call year() on the result it will throw an exception. So basically we don't really need this if block. We just need to call year(). I'm not sure which way is cleaner so please comment. -- To view, visit http://gerrit.cloudera.org:8080/7910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4f430caea88b6c33f8050a1984ee0ee32ecb0a1 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
