impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool
Date Fri, 03 Feb 2017 01:23:08 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool
......................................................................


Patch Set 4:

(37 comments)

http://gerrit.cloudera.org:8080/#/c/5811/4/be/src/runtime/buffered-tuple-stream-v2.cc
File be/src/runtime/buffered-tuple-stream-v2.cc:

Line 326:       LOG(INFO) << "Failed to get reservation for " << pages_.front().len();
> I need to remove this log message.
Done


http://gerrit.cloudera.org:8080/#/c/5811/4/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:

PS4, Line 41:  or scratch disk storage
> what does this mean? isn't it always backed by BufferPool (which  may spill
Reworded to reflect that.


PS4, Line 45: the APIs are called from a single thread
> this is saying per BufferTupleStream object, right? maybe clarify that
Done


Line 46: ///
> it'd be nice to explain the point of this abstraction a bit more explicitly
Done


PS4, Line 49: pinned or unpinned
> I think these terms need to be defined relative to the stream as far as wha
Done


PS4, Line 59: page size
> wouldn't it also be a function of tuple size (i.e. tuples per page)?
I don't think this added much - the previous sentence better describes the number of null
indicators.


PS4, Line 77: pages
> pages' buffers.
Done


PS4, Line 80: location in memory
> or you could say "buffer".
Done


PS4, Line 110: in pages_
> in the stream
Done


PS4, Line 111: in pages_
> same
Done


PS4, Line 114: (if read_write is
             : ///   true, then two pages need to be in memory).
> rather than saying this in parenthesis, i think we should pull it up to say
Reworded this part to say that we only need enough reservation for a single read/write buffer.


PS4, Line 117: caller's reservation is insufficient
> does it first try to increase the reservation at this point?
Done


Line 124: /// returned so far from the stream may be freed on the next call to GetNext().
> do we have a jira we can reference as a TODO here to simplify this?
Done


Line 126: /// Manual construction of rows with AllocateRow():
> not for now, but is it possible to unify things so we only have one way to 
Updated to make it clearer that it's called instead of AddRow().


Line 138: ///     this array as new rows are inserted in the page. If we do so, then there
will be
> is that still something we want to do?
No longer relevant with the changes I made to the null indicator layout.


PS4, Line 146: id
> what is 'id'? is this index?
Done


PS4, Line 147: With 8MB pages that is 512GB per stream.
> where does this limit come into play?  is this something that will cause tr
I believe we only use RowIdx currently if the right side of a join has multiple tuples per
row.

I reworked this to drastically simplify it. Instead of RowIdx we just return a pointer to
the start of the data. This avoids all the problems with limits here.


PS4, Line 186: page_len: the size of pages to use in the stream
> what does that mean for large rows?
We need something like a default and max page length. Added a TODO.


PS4, Line 198: of
> off
Done


PS4, Line 203: AddRow
> is it also used before AllocateRow()?
Done


Line 207:   ///     if an error status is returned.
> broken line break
Done


Line 212:   /// sets 'status' to an error if an error occurred.
> hard to parse that sentence.  but, can we simplify this dance now that we h
Updated this comment to be clearer about the possible scenarios.

It's guaranteed to succeed for now. It's actually a bit more complicated for variable-length
pages since that would rely on the client leaving enough slack in the reservation to read
the largest-possible page.


Line 223:   /// tuples.
> same questions. let's think of this interface can be simplified now.
Updated to refer back to the AddRow() comment.


Line 228:   /// been allocated with the stream's row desc.
> is the row valid only as long as the stream remains pinned?
Done


PS4, Line 232: begin reading
> before the first GetNext()?
Done


Line 236:   ///     false if the page could not be pinned and no error was encountered.
> can we explain that in terms of reservations? and like questions above, doe
Done


PS4, Line 240: enough memory
> what does that mean in terms of reservations?  and does it still make sense
Done


Line 253:   };
> this seems like a wrinkle to the class comment. can it be succinctly descri
I think it now covers it (it mentions that it only pins the pages being read and written in
unpinned mode)


PS4, Line 259: must be copied out by the caller
> this seems to contradict the class comment about "needs_deep_copy"
Updated the comment.


PS4, Line 267: *got_rows is false if the stream could not be pinne
> does that still make sense with reservations?  if so, it should be expresse
Reworded


PS4, Line 271: date
> data?
Done


PS4, Line 271: buffers containing page
> are there buffers that don't contain page data?
Reworded, that was confusing.


PS4, Line 286: Returns the byte size of the stream that is currently pinned in memory.
> Is this trying to say:
Done


http://gerrit.cloudera.org:8080/#/c/5811/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 100:   if (ExecEnv::GetInstance()->buffer_pool() != nullptr) {
> Use exec-env directly.
Done


http://gerrit.cloudera.org:8080/#/c/5811/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 29: #include "runtime/tmp-file-mgr.h" // TODO: can we avoid this?
> Remove TODO
Done


Line 121:   Status CheckSpillingEnabled();
> This is currently dead code - can remove it.
Done


http://gerrit.cloudera.org:8080/#/c/5811/4/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

Line 442:   /// TODO: ownership semantics
> Need to fix this TODO
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5811
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message