Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709252330.v8PNUab1000888@ip-10-146-233-104.ec2.internal>
Date: Mon, 25 Sep 2017 23:30:35 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5307=3A_part_1=3A_don=27t_transfer_disk_I/O_buffers_out_of_parquet=0A?=
In-Reply-To: <gerrit.1505517293000.I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2@gerrit.cloudera.org>
References: <gerrit.1505517293000.I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2@gerrit.cloudera.org>
Reply-To: lv@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        tarmstrong@cloudera.com, reviews@impala.incubator.apache.org,
        dhecht@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="lexAV3UX9gw="; charset=UTF-8
archived-at: Mon, 25 Sep 2017 23:30:47 -0000

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

Lars Volker has posted comments on this change=2E ( http://gerrit=2Eclouder=
a=2Eorg:8080/8085 )

Change subject: IMPALA-5307: part 1: don't transfer di=
sk I/O buffers out of parquet
=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 5:

(7 comments)

http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8085/5//COMMIT_MSG
Commit Message:

http://gerrit=2Ecloudera=2Eo=
rg:8080/#/c/8085/5//COMMIT_MSG@7
PS5, Line 7: IMPALA-5307
Can you add a lin=
e at the bottom what the other part(s) would look like?


http://gerrit=2Ec=
loudera=2Eorg:8080/#/c/8085/5//COMMIT_MSG@56
PS5, Line 56: +---------------=
-----+--------------------------------------------------------+------------=
-----------+--------+-------------+------------+------------+--------------=
--+-------------+-------+
Nit: You could make the second column smaller to =
make this more readable, and add a bottom delimiter line to indicate it was=
 truncated on purposed and not by mistake=2E


http://gerrit=2Ecloudera=2Eo=
rg:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner=2Ecc
File be/src/exec/h=
dfs-parquet-scanner=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8085/5/b=
e/src/exec/hdfs-parquet-scanner=2Ecc@245
PS5, Line 245:   context_->Release=
CompletedResources(nullptr, true);
I think it's best to change the whole fi=
le at once, or only change occurrences where necessary=2E This looks like i=
t may be left from a previous patchset=2E


http://gerrit=2Ecloudera=2Eorg:=
8080/#/c/8085/5/be/src/exec/parquet-column-readers=2Eh
File be/src/exec/par=
quet-column-readers=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8085/5/be=
/src/exec/parquet-column-readers=2Eh@476
PS5, Line 476:   Status AllocateUn=
compressedDataPage(
Should we call this "AllocateUncompressedDataBuffer"? O=
therwise it sounds to me like it'll only be needed for uncompressed pages=
=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8085/5/be/src/exec/parquet-co=
lumn-readers=2Eh@477
PS5, Line 477:       int64_t size, const std::string& =
desc, uint8_t** buffer);
Maybe err_desc, err_detail, or detail? "desc" remi=
nds me of descriptors=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8085/5/b=
e/src/exec/parquet-column-readers=2Eh@485
PS5, Line 485: IsStringType
This =
does not say "VarLenStringType" but above in a comment you refer to var-len=
 data=2E Can you clarify one of them?


http://gerrit=2Ecloudera=2Eorg:8080=
/#/c/8085/5/be/src/exec/parquet-column-readers=2Ecc
File be/src/exec/parque=
t-column-readers=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8085/5/be/s=
rc/exec/parquet-column-readers=2Ecc@1075
PS5, Line 1075:               unco=
mpressed_size, "uncompressed variable-length data", &copy_buffer));
DCHECK(=
copy_buffer !=3D nullptr); And maybe initialize it to nullptr, so that it's=
 explicit what the allocation will do=2E


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerr=
it-MessageType: comment
Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d1480=
1b8260d2
Gerrit-Change-Number: 8085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Ar=
mstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@clou=
dera=2Ecom>
Gerrit-Reviewer: Lars Volker <lv@cloudera=2Ecom>
Gerrit-Comment=
-Date: Mon, 25 Sep 2017 23:30:35 +0000
Gerrit-HasComments: Yes

--lexAV3UX9gw=--