Alex Behm has posted comments on this change. Change subject: Preview: IMPALA-4363: Add timestamp validation ......................................................................
Patch Set 2: (2 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 I don't think this will behave quite right with abort_on_error=false. Returning false here will terminate the scan immediately. Line 599: return Status::OK(); > It's set to NULL in the implementation of Setting a slot to NULL means flipping the null bit in the containing tuple. I don't think you're doing that from within TimestampValue::Validate(), and it wouldn't make sense to do it there. Looking a little closer at the code, I think we may have to move this check into ReadValue() instead of ReadSlot(). We need to do the following if validation fails: tuple->SetNull(null_indicator_offset_); -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
