Taras Bobrovytsky has posted comments on this change. Change subject: Preview: IMPALA-4363: Add timestamp validation ......................................................................
Patch Set 2: (9 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()) { > I don't think this will behave quite right with abort_on_error=false. Retur I rewrote this section, and added a comment. Line 492: /// Similar to NeedsCoversion, most column readers never require validation, so > NeedsConversion() Done 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 Done Line 510: DCHECK(false); > If we ever hit this by accident in a release build we might not want to ret I changed the interface, but False is returned instead of true now. Line 594: // Conversion should only happen when this flag is enabled. > I'll delete this comment. Done Line 599: return Status::OK(); > Setting a slot to NULL means flipping the null bit in the containing tuple. Fixed. Null is being set properly now. 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: Done 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? Done Line 308: "File '$0' column '$1' contains invalid timestamp."), > "Invalid" should be qualified a little more. The value is actually out of r Done -- 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
