impala-reviews mailing list archives

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

Mime
View raw message