Tianyi Wang has posted comments on this change. Change subject: IMPALA-5867: Fix bugs parsing 2-digit year ......................................................................
Patch Set 1: (6 comments) Refactored and changed a few logic to make it a little bit closer to Hive. 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? Done Line 345: if (dt_result.realign_year) { > This function is getting pretty big. Can we factor out the year realignment Done Line 346: // In SimpleDateFormat, the century for 2-digit-year breaks at current_time - 80 years. > Long line. Done PS1, Line 353: boost::gregorian::gregorian_calendar > This might be clearer with a "using boost::gregorian::gregorian_calendar" u Done Line 370: if (d->year() < 1400 || d->year() > 9999) { > Boost still won't throw an exception when you add -1 days to 1400-01-01. Bu Changed to a single call to year(). It looks stupid but I don't know other ways to check if it's in range. 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. Done -- 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
