impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3344: Simplify sorter and document/enforce invariants.
Date Tue, 03 May 2016 05:21:33 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3344: Simplify sorter and document/enforce invariants.
......................................................................


Patch Set 5:

(14 comments)

I had started on this but haven't made it through, but flushing out my comments since I see
MJ gave you a review too so you can include these in the next round.

http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorted-run-merger.cc
File be/src/runtime/sorted-run-merger.cc:

Line 70:         RETURN_IF_ERROR(sorted_run_(&input_row_batch_));
don't we need to transfer ownership for any empty batches? i.e. does L64-66 need to be in
this loop?
But how does that work -- couldn't an empty batch be AtCapacity(), and if so don't we need
to not consume more of them?
Also, why is the loop needed now, whereas it was a DCHECK before?


http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorted-run-merger.h
File be/src/runtime/sorted-run-merger.h:

Line 47:   /// zero).
is that now true with the loop you added?


http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

Line 48: blocks
'blocks'


Line 51: block != NULL
why are there NULL blocks?


Line 58: two
this directly contradicts the first sentence. let's reword that - the first sentence could
just describe a run conceptually, and the second is the implementation.


Line 59: and
"that may contain"?


Line 61: using a merger
either delete or reference the actual class name. Otherwise, this isn't very helpful to a
reader.


Line 90: last
final


Line 106: The callee (Run)
This Run


Line 116: Atmost
At most


Line 118: all rows in output_batch will have their fixed and var-len data from
        :   /// the same block
what? doesn't var-len data live in a separate block from fixed?


Line 188: tuple
'tuple'


Line 189: var_len_blocks_index_
'


http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/util/tuple-row-compare.h
File be/src/util/tuple-row-compare.h:

Line 99:     return false;
return result < 0;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c619e81fd1b8ac50e257172c8bce101a112b52a
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message