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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-HasComments: Yes
