impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pooja Nilangekar (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Build a ConcatenatedStreams wrapper for ScannerContext::Stream
Date Thu, 27 Jul 2017 21:46:31 GMT
Pooja Nilangekar has posted comments on this change.

Change subject: Build a ConcatenatedStreams wrapper for ScannerContext::Stream
......................................................................


Patch Set 2:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/7513/1//COMMIT_MSG
Commit Message:

Line 16: 
> It's best to document the TODO in the ConcatenatedStreams class itself, sin
Done


http://gerrit.cloudera.org:8080/#/c/7513/1/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:

PS1, Line 330:   SkipStreamIfEosr();
> Is there a reason why you cannot DCHECK here, too?
Done


PS1, Line 346:   uint8_t** buffer, int64_t* out_len, Status* status, bool peek) {
             :   SkipStreamIfEosr();
             :   r
> This block looks like a candidate for a method that increments current_idx_
Done


http://gerrit.cloudera.org:8080/#/c/7513/1/be/src/exec/scanner-context.h
File be/src/exec/scanner-context.h:

PS1, Line 219: boundary_buffer_
> This buffer looks like a good candidate in case you want to stitch together
Should I look into this in a separate change?


PS1, Line 273: Concatenates a list of streams by reading them sequentia
> "Concatenates a list of streams by reading them sequentially." ?
Done


PS1, Line 274: s a wrapper around the functions which are
             :   /// utilized by BaseScalarColumnReader.
> This is a detail of how the class can be used. Try to describe only what it
Done


Line 287: 
> pass by reference
The scope of the vector is causing issues for call by reference.


Line 290: 
> I don't think you need to explicitly define it as empty, since that's the d
Done


PS1, Line 295: ed_len byt
> missing word?
Done


PS1, Line 299: 
> I think you can just say "the current stream", here and elsewhere.
Current stream could be misleading since incase the current stream has hit eosr, the current
index is advanced to ensure that the bytes are read from a following stream which has not
hit eosr.


PS1, Line 300: 
> Shouldn't it say "the last stream" since you're concatenating them in order
The current index would reach the last stream only if every stream before it has hit eosr.
So I was thinking 'every stream' makes it clear.


PS1, Line 313: whic
> from?
Done


Line 317:     int current_idx_;
> If you already know all the streams when creating the class, then you could
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I026f7a005ba4d6fc412ca0884f4fa0459da9c885
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pooja Nilangekar <pooja.nilangekar@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <pooja.nilangekar@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message