Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4363: Add Parquet timestamp validation ......................................................................
Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/4968/8/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 473: ErrorMsg* msg = NULL; > see comment below. this pointer is never freed. what's the objection to pu Ok, I agree, it's better to put the logic inside ValidateSlot(). Line 484: // This point is reached if slot validation succeeds, but slot conversion fails. > but don't we need to validate the converted value? i.e. the timestamp might Convert slot should never return an invalid value. I tried it manually and it works correctly. Line 603: *msg = new ErrorMsg(TErrorCode::PARQUET_TIMESTAMP_OUT_OF_RANGE, > this memory is leaked. This is not a problem any more. http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: PS7, Line 160: g buffer. The size of the buffer should be a > What I'm asking is: don't some days legitimately have more than NUMBER_OF_N Yes, that's true, some days can have an extra second: https://en.wikipedia.org/wiki/Leap_second So far there hasn't been a case where a day has 2 leap seconds, but it's not impossible in the future. If we do this, the way seconds are printed to screen would be incorrect, Impala would print "24:00:00" instead of "23:59:60". Also, we would need to be able to cast string as timestamp that looks like "23:59:60" if we want to support leap seconds. I'm not really sure what's the right solution here. Should we just increase the constant by 1? By the way, even Oracle doesn't handle leap seconds properly (the number of seconds can't be greater that 59): http://stackoverflow.com/questions/31136211/how-to-handle-leap-seconds-in-oracle -- 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: 8 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-HasComments: Yes
