impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5867: Fix bugs parsing 2-digit year
Date Thu, 31 Aug 2017 16:26:14 GMT
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 <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message