Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4363: Add Parquet timestamp validation ......................................................................
Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 473: ErrorMsg msg; > this has a non-trivial destructor, so we should probably only construct it I added some comments to make the code more clear. It doesn't seem elegant to pass tuple into ValidateSlot, so I fixed the problem with the non-trivial destructor differently. PS7, Line 474: val_ptr > you'll also have to be careful that we pass the right pointer here, dependi I think I'm passing it correctly in the latest patch. http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: Line 147: /// Verifies that the timestamp date falls into a valid range (years 1400..9999). > update this comment to be clear that we also validate time. Done PS7, Line 156: l > doesn't this have to be "ll" (long long)? Yes, that's true (it's 64 bits). PS7, Line 160: time_.ticks() < NUMBER_OF_NANOSECONDS_IN_24H > I'm slightly worried about how this interacts with leap seconds. Are you s The result of a select statement would look like, for example: 1995-05-05 54:35:31 -- 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: 7 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
