Dan Hecht 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 putting the logic inside ValidateSlot() so that the memory allocation can stay automatic? 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 be within bounds before conversion, but not after. do we handle that? Line 603: *msg = new ErrorMsg(TErrorCode::PARQUET_TIMESTAMP_OUT_OF_RANGE, this memory is leaked. 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 > The result of a select statement would look like, for example: What I'm asking is: don't some days legitimately have more than NUMBER_OF_NANOSECNODS_IN_24H when leap seconds are in play? -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
