Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710252235.v9PMZrJD017093@ip-10-146-233-104.ec2.internal>
Date: Wed, 25 Oct 2017 22:35:53 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Tianyi Wang <twang@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>,
        Thomas Tauber-Marshall <tmarshall@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2758=3A_Remove_BufferedTupleStream=3A=3AGetRows=0A?=
In-Reply-To: <gerrit.1507310620000.I3831c38994da2b69775a9809ff01de5d23584414@gerrit.cloudera.org>
References: <gerrit.1507310620000.I3831c38994da2b69775a9809ff01de5d23584414@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com, twang@cloudera.com,
        marcelk@gmail.com, dhecht@cloudera.com, tmarshall@cloudera.com,
        reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="6yAPBwVDH14="; charset=UTF-8
archived-at: Wed, 25 Oct 2017 22:35:59 -0000

--6yAPBwVDH14=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Tim Armstrong has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/8226 )

Change subject: IMPALA-2758: Remove BufferedTupleStr=
eam::GetRows
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=


Patch Set 2:

(5 comments)

The code change looks good, just needs a bit=
 more cleanup=2E

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/exe=
c/partitioned-hash-join-node=2Eh
File be/src/exec/partitioned-hash-join-nod=
e=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/exec/partitio=
ned-hash-join-node=2Eh@482
PS2, Line 482: output_null_aware_probe_rows_runn=
ing
Needs underscore on end=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/82=
26/2/be/src/exec/partitioned-hash-join-node=2Ecc
File be/src/exec/partition=
ed-hash-join-node=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/=
src/exec/partitioned-hash-join-node=2Ecc@948
PS2, Line 948:     if (!got_re=
servation) return NullAwareAntiJoinError(null_build_stream);
The stream is =
pinned so this should be impossible - let's make it a DCHECK=2E


http://ge=
rrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/exec/partitioned-hash-join-nod=
e=2Ecc@1073
PS2, Line 1073:       if (!got_reservation) return NullAwareAnt=
iJoinError(build);
Same here - got_reservation should always be true for a =
pinned stream=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/ru=
ntime/buffered-tuple-stream=2Eh
File be/src/runtime/buffered-tuple-stream=
=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/runtime/buffer=
ed-tuple-stream=2Eh@127
PS2, Line 127: or GetRows()
There are some referenc=
es to GetRows() in the comments that can be cleaned up too=2E


http://gerr=
it=2Ecloudera=2Eorg:8080/#/c/8226/2/be/src/runtime/buffered-tuple-stream=2E=
h@371
PS2, Line 371:   friend class SimpleTupleStreamTest_TestGetRowsOverfl=
ow_Test;
This test was removed too=2E


-- 
To view, visit http://gerrit=
=2Ecloudera=2Eorg:8080/8226
To unsubscribe, visit http://gerrit=2Ecloudera=
=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerr=
it-MessageType: comment
Gerrit-Change-Id: I3831c38994da2b69775a9809ff01de5d=
23584414
Gerrit-Change-Number: 8226
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi=
 Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2E=
com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera=2Ecom>
Ger=
rit-Reviewer: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armst=
rong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Wed, 25 Oct 2017 22:3=
5:53 +0000
Gerrit-HasComments: Yes

--6yAPBwVDH14=--