Alex Behm has posted comments on this change. Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter(). ......................................................................
Patch Set 5: (1 comment) It does seem like this is not a regression at least in this code, but probably elsewhere, so I'm happy to abandon this if so desired. It's still unclear to me why the perf runs started failing though. Those bad files have been there forever. http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg()) > Right, this won't work as was discussed in the earlier comment. I maintain this will work, and I did run through these paths on ASAN. 1. column_readers_ are set inside NextRowGroup() and are only accessed of there is a valid row group to scan 2. if ProcessFooter() exits before setting file_metadata_, then we have a file_metadata_ with 0 rows and no row groups, so we will cleanly exit out of GetNextInternal(), see L313 and L351, and never set or access the column readers I added DCHECKs to show what state the file_metadata_ is in before calling into ProcessFooter(). There is still a discussion to be had on whether we should be able to continue on all of the errors in ProcessFooter(), e.g., if there's an unexpected file version. If you'd prefer to add a special flag like open_ok for clarity that's fine, but not necessary. Dan, the commit you are looking for is: 8343656189d631491b7372a60e228c851b61165c -- To view, visit http://gerrit.cloudera.org:8080/3862 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
