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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
