Lars Volker has posted comments on this change. Change subject: Preview: IMPALA-4363: Add timestamp validation ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/4968/1/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 44: ? Line 491: inline bool NeedsValidation() const { Can you add a brief comment? Line 502: /// Verifies that data is acceptable Can you elaborate what "acceptable" means? http://gerrit.cloudera.org:8080/#/c/4968/1/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: Line 101: void TimestampValue::Validate() { Can you move the exception handling into DebugString()? This way it would be more obvious to see there, that we handle the exception. And we would be less prone to adding new calls to DebugString() that are not protected by Validate. PS1, Line 103: Nit: tab. git-clang-format should fix this. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
