Michael Ho has posted comments on this change. Change subject: IMPALA-3115: Hoist some variables out of loops in cross-compiled code. ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/2661/6/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 53: next_probe_row > would it be simpler to not have this variable, but instead just use probe_b Done Line 183: next_probe_row; > i.e. this could be probe_batch_iterator.Get() Done http://gerrit.cloudera.org:8080/#/c/2661/6/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 164: row_ = NULL; > why do we have to special case the full case? can't we just require that ro It was done so that we won't hit the DCHECK() in GetRow(). As we discussed in person, I will duplicate the logic of GetRow() here instead. Also added some DCHECKs for sanity check. Line 170: TupleRow* IR_ALWAYS_INLINE Next() { > add function comments for these methods. Done Line 172: DCHECK(row_ == NULL || > row_ can't be NULL here. It can before as we also set num_tuples_per_row_ to 0. It's irrelevant in the new patch now. Line 173: (row_ - parent_->tuple_ptrs_) / num_tuples_per_row_ <= parent_->capacity_); > Isn't this the same as DCHECK_LE(row_, row_batch_end_)? Not really. This is testing against the capacity of the row batch which is useful for write iterator. Line 398: > Why not have the syntax closer to BOOST_FOREACH? Done. -- To view, visit http://gerrit.cloudera.org:8080/2661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7152b1fb094b3c3574d203e3774f4297f2225dc Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
