impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
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:


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.
File be/src/runtime/

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?
File be/src/runtime/sorted-run-merger.h:

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

Line 48: 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

Line 90: last

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

Line 189: var_len_blocks_index_
File be/src/util/tuple-row-compare.h:

Line 99:     return false;
return result < 0;

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c619e81fd1b8ac50e257172c8bce101a112b52a
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message