impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Date Thu, 28 Sep 2017 20:51:44 GMT
Bharath Vissapragada has posted comments on this change. (

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

Patch Set 2:


Didn't see the test class, but got some comments on the logic.
File be/src/service/
PS2, Line 450: // whether to compress or decompress the 'src' into 'dst'.
May be worth commenting about GetPrimitiveArrayCritical() and lifecycle of the buffer. Looks
like it is up to the JVM to GC it eventually.
PS2, Line 451: extern "C"
Is extern required for this method? Also, no return type?
PS2, Line 453:     jbyteArray dst, jint dst_off, jint dst_len, jboolean compress) {
DCHECK_GE(dst_len, LZ4_compressBound(src_size))?
PS2, Line 454: 0
nit: Not sure if this was intentional, but you are passing a nullptr argument. JDK can handle
it ok [1], but looks like you need to pass a pointer to 0/1. 

Also, we should probably make sure the (isCopy==false) (after the call), incase the JVM does
a copy for some reason, we should free the copied native bytes.

PS2, Line 455:   if (src_data == nullptr) return -1;
Do we still need to call ReleasePrimitiveArrayCritical() in this case? Also, I see that you
implemented a ScopedArrayCritical class to do that (much cleaner). Wondering why you've undone
the changes? (same below)
File fe/src/main/java/org/apache/impala/service/
PS2, Line 285:    */
Probably worth commenting about the format of the output buffer.
PS2, Line 289: if (dstLen <= 0) {
             :       throw new InternalException(
             :           "Payload is too big for LZ4 compression: " + payload.length);
             :     }
Move this below L294?
PS2, Line 306: ByteBuffer.wrap(dst).putInt(0, payload.length);
             :     ByteBuffer.wrap(dst).putInt(4, compressedSize);
Can't we return the resized buffer directly here? Basically merge the logic of Lz4CompressToByteBuffer
into this method?

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: 2
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Comment-Date: Thu, 28 Sep 2017 20:51:44 +0000
Gerrit-HasComments: Yes

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