impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3344: Simplify sorter and document/enforce invariants.
Date Wed, 04 May 2016 23:27:01 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(8 comments)

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

Line 33: BatchedRowSupplier
> A few high level things:
I couldn't think of anything much clearer. Done.

2) See my earlier comment - I think the right thing to do here is to avoid handling need_to_return()
and simplify this. 

I think the three general alternatives here are a) copy the rows so they don't reference anything
passed up from the sorter, b) do the complicated TryAdvance() handshake so that we can free
resources before moving to the next row or c) use double the resources while transitioning
between blocks.

It doesn't seem like the copying is a major performance bottleneck right now, so it's probably
the best approach until it shows up on profiles.


Line 55: exhausted
> exhausted,
Done


Line 57: resouce
> resources
Done


Line 59: Next
> Per my larger comment at the top, how about:
I think 1) and 2) aren't relevant any more since I removed handling of the need_to_return
case.

I did rename Next() to Advance().

3) is done.


Line 86: there is at least one row remaining in the batch.
> ...at least one row (including current_row()) remaining in the current batc
Done


Line 87: before 'done' is returned.
> before Next() returns with 'done' set to true
Done


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

Line 69: If 'deep_copy_input_' is false
> If true, this should always be able to advance, right? Can you maybe add th
With the other changes this got simplier.


Line 78: AdvanceMinRow
> TryAdvanceMinRow?
See above.


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