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 Mon, 02 Oct 2017 23:49:16 GMT
Alex Behm has posted comments on this change. ( )

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

Patch Set 5:

File be/src/service/
PS5, Line 457: // It does not matter if 'src' is copied because we only read from it.
> nit: May be document src_off, dst_off and return value for readability.
Documented return values.

I think the 'off' and 'len' params are standard enough to not need extra comments.
PS5, Line 481:     env->SetByteArrayRegion(dst, 0, len, reinterpret_cast<jbyte*>(dst_data));
> Check the return value of this? Given it made a copy, its likely that this 
Function returns void.

Added code to handle exceptions.
File be/src/util/jni-util.h:
PS5, Line 184: !is_copy_ &&
> My understanding is that we should call ReleasePrimitiveArrayCritical() irr
Good catch, that was a bug. Fixed.
PS5, Line 192: ;

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: 5
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Comment-Date: Mon, 02 Oct 2017 23:49:16 +0000
Gerrit-HasComments: Yes

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