Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8814 )
Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.h@278 PS6, Line 278: buffer nit: buffers http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.h@286 PS6, Line 286: /// to point at the boundary buffer variables. Returns an error if the boundary This function also advances io_buffer_pos_ and io_buffer_bytes_left_, right ? http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.cc@239 PS6, Line 239: if (boundary_buffer_bytes_left_ >= requested_len) break; Not sure I understand why this is needed in the new change ? Wouldn't the while loop condition imply that this is not true ? http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.inline.h File be/src/exec/scanner-context.inline.h: http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.inline.h@70 PS6, Line 70: SkipBytes( This implementation of SkipBytes is more efficient than GetBytes() as it avoids both the memcpy overhead into boundary_buffer and the memory allocation needed to hold the skipped bytes, right ? http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.inline.h@84 PS6, Line 84: if (bytes_left == 0) return true; So, if boundary_buffer_bytes_left_ == 0 && io_buffer_bytes_left_ == 0 at this point, I assume output_buffer_bytes_left_ has to be 0 as output_buffer_bytes_left_ should be pointing to boundary_buffer_bytes_left_, right ? Can we add the following at line 74 DCHECK(output_buffer_pos_ == boundary_buffer_pos_); DCHECK(output_buffer_bytes_left_ == boundary_buffer_bytes_left_) ? http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.inline.h@187 PS6, Line 187: *buffer_pos += bytes; DCHECK_LE(bytes, *buffer_bytes_left); -- To view, visit http://gerrit.cloudera.org:8080/8814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0 Gerrit-Change-Number: 8814 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 09 Jan 2018 22:20:01 +0000 Gerrit-HasComments: Yes
