impala-reviews mailing list archives

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

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


Patch Set 1:

(2 comments)

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

PS1, Line 335: DCHECK(ValidateCurrentIndex());
Just a passing comment - if this DCHECK fails, all it will tell us via the logs is that ValidateCurrentIndex()
was false, because the DCHECK will just print the condition that failed. 

This is useful for debugging, but not as useful as knowing *how* it failed (off by one error?
was the idx badly initialized?). So my suggestion is to have ValidateCurrentIndex() look more
like this:

   void ValidateCurrentIndex() {
     DCHECK_LE(0, current_idx_);
     DCHECK_GT(streams_.size(), current_idx_);
   }

The compiler will figure out in release mode that the method call does nothing, and should
be able to optimize it out.


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

PS1, Line 332: return current_idx_
consider case where current_idx_ < 0 as well?


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pooja Nilangekar <pooja.nilangekar@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <pooja.nilangekar@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message