Tim Armstrong has posted comments on this change. Change subject: IMPALA-5867: Fix bugs parsing 2-digit year ......................................................................
Patch Set 1: (8 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 340 Can you preserve this comment? I think you moved this logic down to l.370? Line 68: century_break_ptime = boost::posix_time::ptime( Mention that we set it to March 1 for consistency with Hive? Line 345: if (dt_result.realign_year) { This function is getting pretty big. Can we factor out the year realignment logic? Line 346: // In SimpleDateFormat, the century for 2-digit-year breaks at current_time - 80 years. Long line. Line 352: if (dt_result.month == 2 && dt_result.day == 29 && I don't fully understand this condition. Couldn't this advance 100 years even if the date is after the century break? E.g. if the century break is 1900-01-01 and the date is 00-02-29, then I think this would give 2000-01-01, but I think 1900-02-29 would be correct. I think the only years where y is not a leap year but y + 100 is a leap year are 1900, 2300, 2700 (since years divisible by 100 are not leap years, except for years divisible by 400) so this may not be that important in practice but it would be good to ensure the logic is correct and have a comment explaining it. PS1, Line 353: boost::gregorian::gregorian_calendar This might be clearer with a "using boost::gregorian::gregorian_calendar" up the top of the file. 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 in boost where an exception wasn't thrown in some edge cases. May be worth seeing if the expr-tests pass without this. http://gerrit.cloudera.org:8080/#/c/7910/1/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: Line 484: (TimestampTC("yy-MM-dd", "00-02-29", false, true, false, 2000, 2, 29)) Mention this is testing the leap-year edge-case. -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
