Dan Hecht 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? PS10, Line 155: must why "must"? or do you mean "can't"? 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 that case? PS10, Line 200: DISK_IO_ERROR why not INTERNAL_ERROR, since that's the code that indicates an internal error (i.e. bug). 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 read. How about applying deMorgan's to remove the double negation: return boundary_buffer_bytes_left_ == 0 || (output_buffer_pos_ == &boundary_buffer_pos_ && output_buffer_bytes_left == &boundary_buffer_bytes_left_); Alternatively, it may be even easier to read if you just put the DCHECK in there: if (boundary_buffer_bytes_left_ > 0) { DCHECK_EQ(output_buffer_pos_, &boundary_buffer_pos_); DCHECK_EQ(output_buffer_bytes_left_, &boundary_buffer_bytes_left_); } (which you could also use at line 228-229). Also, is there an invariant for the "else" case w.r.t. io_buffer* that makes sense to check? -- 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 <l...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
