Tim Armstrong 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: (7 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 Done 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 Done 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@99 PS6, Line 99: done || > It appears that done == true implies io_buffer_bytes_left_ == 0 in the new That's only true if the caller read all the way to the end of the stream. That's not true if it hit an error or was cancelled early. 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 w CopyIoToBoundary() copies over some or all of the I/O buffer increments boundary_buffer_bytes_left_. We don't want to get a new I/O buffer in the case where we copied over only part of the I/O buffer. 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 av Yeah, exactly. It also seemed dangerous that skipping N bytes had the unexpected side-effect of allocating N bytes of data - callers of the API wouldn't expect that. 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 th Done 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); Done -- 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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 09 Jan 2018 23:06:07 +0000 Gerrit-HasComments: Yes
