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 Thu, 31 Aug 2017 18:37:25 GMT
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 <twang@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message