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-4363: Add Parquet timestamp validation
Date Thu, 01 Dec 2016 01:56:13 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4363: Add Parquet timestamp validation
......................................................................


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4968/9/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:

Line 657: 
Unnecessary blank line.


http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

Line 151:     const static int64_t NUMBER_OF_NANOSECONDS_IN_24H =
boost provides a direct way to construct the min and max date with 


  boost::gregorian::date(boost::date_time::min_date_time)
  boost::gregorian::date(boost::date_time::max_date_time)


PS7, Line 160: g buffer. The size of the buffer should be a
> Yes, that's true, some days can have an extra second: https://en.wikipedia.
It looks like boost date_time doesn't support leap seconds: https://groups.google.com/forum/#!topic/boost-list/i_0h_amkk-c

"Leap second support never got added to the library -- most applications can ignore them.
 In principle, of course, you can use the library to help you do the calculations.  Basically
they are like leap years except that they are irregular -- so you have to use a collection
to perform the adjustment instead of an algorithm."

I think we should allow for one leap second per day just so we don't regress anything where
the data is potentially valid. It would be good to add some testing of this (maybe as a follow)
to make sure we don't crash, even if we do return bogus results.


-- 
To view, visit http://gerrit.cloudera.org:8080/4968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message