Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710262222.v9QMMYSY013123@ip-10-146-233-104.ec2.internal>
Date: Thu, 26 Oct 2017 22:22:34 +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="Ub3JxO9qeLo="; charset=UTF-8
archived-at: Thu, 26 Oct 2017 22:22:40 -0000

--Ub3JxO9qeLo=
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 4: Code-Review+2

(4 comments)

Looks good, just did a final p=
ass and caught a couple more things=2E If you fix those and rebase then I c=
an start the merge=2E

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/4/be/sr=
c/exec/partitioned-hash-join-node=2Eh
File be/src/exec/partitioned-hash-joi=
n-node=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/4/be/src/exec/par=
titioned-hash-join-node=2Eh@482
PS4, Line 482:   bool output_null_aware_pro=
be_rows_running_;
Longer-term we should think about ways to reduce the shee=
r number of stateful member variables in this class and make the state mach=
ine in GetNext() easier to understand=2E


http://gerrit=2Ecloudera=2Eorg:8=
080/#/c/8226/4/be/src/exec/partitioned-hash-join-node=2Ecc
File be/src/exec=
/partitioned-hash-join-node=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/=
8226/4/be/src/exec/partitioned-hash-join-node=2Ecc@949
PS4, Line 949:     R=
owBatch null_build_batch(child(1)->row_desc(), state->batch_size(),
This ca=
n fit on one line=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/4/be/sr=
c/exec/partitioned-hash-join-node=2Ecc@962
PS4, Line 962:       null_build_=
batch=2EReset();
We should add a RETURN_IF_CANCELLED() check here, similar =
to the one below (I think this was an oversight in an earlier patch)=2E


h=
ttp://gerrit=2Ecloudera=2Eorg:8080/#/c/8226/4/be/src/exec/partitioned-hash-=
join-node=2Ecc@1074
PS4, Line 1074:       RowBatch build_batch(child(1)->ro=
w_desc(), state->batch_size(),
This can fit on one line=2E


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

Gerrit-Project: Impala-ASF
Gerrit-B=
ranch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3831c38994da2b=
69775a9809ff01de5d23584414
Gerrit-Change-Number: 8226
Gerrit-PatchSet: 4
Ge=
rrit-Owner: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <=
dhecht@cloudera=2Ecom>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@c=
loudera=2Ecom>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-R=
eviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Thu=
, 26 Oct 2017 22:22:34 +0000
Gerrit-HasComments: Yes

--Ub3JxO9qeLo=--