Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201605042327.u44NR1Vu009915@ip-10-146-233-104.ec2.internal>
Date: Wed, 4 May 2016 23:27:01 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?[Impala-CR](cdh5-trunk)_IMPALA-3344:_Simplify_sorter_and_document/enforce_invariants.=0A?=
In-Reply-To: <gerrit.1461190325000.I9c619e81fd1b8ac50e257172c8bce101a112b52a@gerrit.cloudera.org>
References: <gerrit.1461190325000.I9c619e81fd1b8ac50e257172c8bce101a112b52a@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0
archived-at: Wed, 04 May 2016 23:27:08 -0000

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