Alex Behm has posted comments on this change. Change subject: Preview: IMPALA-4363: Add timestamp validation ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/4968/2/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 473: if (NeedsValidation() && !ValidateSlot(val_ptr).ok()) { single line if it fits should this be applied before or after the timestamp conversion? add comment Line 492: /// Similar to NeedsCoversion, most column readers never require validation, so NeedsConversion() Line 507: /// Ensures the data is valid. If it is discovered that data is not valid, Sets the slot to NULL if the slot value is invalid, e.g., due to being out of the valid value range. Line 599: return Status::OK(); Where is the slot set to NULL? http://gerrit.cloudera.org:8080/#/c/4968/2/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: Line 146: bool Validate(); Can you change this to something like: inline bool IsValidDate() const { // Implementation goes here, so it can be inlined. } Needs brief comment, ideally, containing text describing the min/max values. Also, better not change *this, i.e.,a void having side effects. http://gerrit.cloudera.org:8080/#/c/4968/2/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: Line 307: ("PARQUET_TIMESTAMP_INVALID", 100, PARQUET_TIMESTAMP_OUT_OF_RANGE? Line 308: "File '$0' column '$1' contains invalid timestamp."), "Invalid" should be qualified a little more. The value is actually out of range. Might be good to include the min/max range in the error message. -- 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: 2 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-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
