Lars Volker has posted comments on this change. Change subject: IMPALA-5890: Abort queries if scanner hits IO errors ......................................................................
Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/8011/10/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: PS10, Line 154: if the scan range has returned eosr before > if that was the case, why can we have !eosr at line 152? I think this was an old comment and I couldn't convince myself anymore that it was correct. I also discovered that the DCHECK should read (!status.ok || ...) because if the status is OK, that implies the buffer must not be NULL). PS10, Line 155: must > why "must"? or do you mean "can't"? Yes, can't is what I meant to say. Removed the comment though. PS10, Line 155: this is the first time the function : // is called > why is that significant? and why is it okay to have io_buffer_==NULL in tha Removed the comment. PS10, Line 200: DISK_IO_ERROR > why not INTERNAL_ERROR, since that's the code that indicates an internal er I can change that, but then we also have to abort the query in GetNextInternal() on INTERNAL_ERROR. I think we re-used DISK_IO_ERROR here to keep the set of aborting errors small. Would you prefer to switch to INTERNAL_ERROR? PS10, Line 351: if (boundary_buffer_bytes_left_ > 0 && : (output_buffer_pos_ != &boundary_buffer_pos_ || : output_buffer_bytes_left_ != &boundary_buffer_bytes_left_)) { : return false; > this is effectively a double negative statement, which makes it harder to r Done. I think in the else case (boundary_buffer_bytes_left_ == 0) the buffer pointers could point to either buffer. -- To view, visit http://gerrit.cloudera.org:8080/8011 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509 Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
