impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Date Sat, 30 Sep 2017 04:03:47 GMT
Alex Behm has posted comments on this change. ( )

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression

Patch Set 3:

File be/src/service/
PS3, Line 452: the
> remove
PS3, Line 458: Note that it does not matter if 'src' was copied
> Why don't we treat it as an error? This could mean an OOM on the JVM?
We only read the source, so we don't care if we get the bytes directly or a copy. We write
into 'dst' and writing into a copy is not useful because then the caller has no data in the
real 'dst'.

Added comment and code to handle the is_copied case instead of returning an error.
PS3, Line 471: "
> May be better to add that the JVM could've run out of memory (supportabilit
PS3, Line 472:     env->ReleasePrimitiveArrayCritical(src, src_data, 0);
I think there was a subtle bug here with not calling ReleasePrimitiveArrayCritical() on the
dst if it was copied.

I restored the scoped array critical class.
File fe/src/main/java/org/apache/impala/service/
PS3, Line 306:     if (compressedSize < 0) {
> Looks like this check should be <=.
File fe/src/test/java/org/apache/impala/service/
PS3, Line 66:     } catch (Throwable e) {
> I think fail() throws an "Error" (AssertionError) which this Throwable can 
Reworked. Still want to catch Throwable here to check for OOM. See test in L162.
PS3, Line 72: e.printStackTrace();
> unreachable?
PS3, Line 78:   private byte[] seqPayload(int numBytes) {
> Add one liner comments on these helpers.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Comment-Date: Sat, 30 Sep 2017 04:03:47 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message