impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] Preview: IMPALA-4363: Add timestamp validation
Date Tue, 08 Nov 2016 02:19:51 GMT
Alex Behm has posted comments on this change.

Change subject: Preview: IMPALA-4363: Add timestamp validation

Patch Set 2:

File be/src/exec/

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

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

Line 599:   return Status::OK();
Where is the slot set to NULL?
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.
File common/thrift/


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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message