Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710022349.v92NnGXn022944@ip-10-146-233-104.ec2.internal>
Date: Mon, 2 Oct 2017 23:49:16 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Philip Zeyliger <philip@cloudera.com>,
        Bharath Vissapragada <bharathv@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5990=3A_Part_1=3A_JNI-based_LZ4_de/compression=0A?=
In-Reply-To: <gerrit.1506489450000.I237802944875b07080db0159ff8ec548150fd95e@gerrit.cloudera.org>
References: <gerrit.1506489450000.I237802944875b07080db0159ff8ec548150fd95e@gerrit.cloudera.org>
Reply-To: alex.behm@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        philip@cloudera.com, bharathv@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="EVncEufDazo="; charset=UTF-8
archived-at: Mon, 02 Oct 2017 23:49:22 -0000

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

Alex Behm has posted comments on this change=2E ( http://gerrit=2Ecloudera=
=2Eorg:8080/8150 )

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/c=
ompression
=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:

(4 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8150/5/=
be/src/service/fe-support=2Ecc
File be/src/service/fe-support=2Ecc:

http:/=
/gerrit=2Ecloudera=2Eorg:8080/#/c/8150/5/be/src/service/fe-support=2Ecc@457=

PS5, Line 457: // It does not matter if 'src' is copied because we only re=
ad from it=2E
> nit: May be document src_off, dst_off and return value for =
readability=2E
Documented return values=2E

I think the 'off' and 'len' par=
ams are standard enough to not need extra comments=2E


http://gerrit=2Eclo=
udera=2Eorg:8080/#/c/8150/5/be/src/service/fe-support=2Ecc@481
PS5, Line 48=
1:     env->SetByteArrayRegion(dst, 0, len, reinterpret_cast<jbyte*>(dst_da=
ta));
> Check the return value of this? Given it made a copy, its likely th=
at this 
Function returns void=2E

Added code to handle exceptions=2E


htt=
p://gerrit=2Ecloudera=2Eorg:8080/#/c/8150/5/be/src/util/jni-util=2Eh
File b=
e/src/util/jni-util=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8150/5/be=
/src/util/jni-util=2Eh@184
PS5, Line 184: !is_copy_ &&
> My understanding i=
s that we should call ReleasePrimitiveArrayCritical() irr
Good catch, that =
was a bug=2E Fixed=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8150/5/be/s=
rc/util/jni-util=2Eh@192
PS5, Line 192: ;
> WARN_UNUSED_RESULT
Done


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

Gerrit-Project: Impala-ASF=

Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I23780=
2944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-Patch=
Set: 5
Gerrit-Owner: Alex Behm <alex=2Ebehm@cloudera=2Ecom>
Gerrit-Reviewer=
: Alex Behm <alex=2Ebehm@cloudera=2Ecom>
Gerrit-Reviewer: Bharath Vissaprag=
ada <bharathv@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <philip@clou=
dera=2Ecom>
Gerrit-Comment-Date: Mon, 02 Oct 2017 23:49:16 +0000
Gerrit-Has=
Comments: Yes

--EVncEufDazo=--