Michael Ho has posted comments on this change. Change subject: IMPALA-3756: Fix wrong argument type in HiveStringsTest ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/3617/5/fe/src/main/java/com/cloudera/impala/hive/executor/UdfExecutor.java File fe/src/main/java/com/cloudera/impala/hive/executor/UdfExecutor.java: PS5, Line 102: // Allocations made from the native heap that need to be cleaned when this object : // is GC'ed. : private final ArrayList<Long> allocations_ = Lists.newArrayList(); > not needed Done PS5, Line 236: for (long ptr: allocations_) { : UnsafeUtil.UNSAFE.freeMemory(ptr); : } : allocations_.clear(); > not needed Done http://gerrit.cloudera.org:8080/#/c/3617/5/fe/src/test/java/com/cloudera/impala/hive/executor/UdfExecutorTest.java File fe/src/test/java/com/cloudera/impala/hive/executor/UdfExecutorTest.java: PS5, Line 199: void > is this needed elsewhere in the package? if not, make this private The default scope is package private. I keep it as-is to be consistent with the rest of the methods in the class. PS5, Line 200: (w instanceof ImpalaBooleanWritable || : w instanceof ImpalaTinyIntWritable || : w instanceof ImpalaSmallIntWritable || : w instanceof ImpalaIntWritable || : w instanceof ImpalaBigIntWritable || : w instanceof ImpalaFloatWritable || : w instanceof ImpalaDoubleWritable || : w instanceof String) > I don't really care but we don't do this kind of cool formatting. Might be Ha. I removed the space and rearranged the statements to make it more legible. PS5, Line 216: Empty string means using the current : * jar file > I thought the behavior of taking null rather than an empty string made more Apparently, 'local_location' is a required field in THiveUdfExecutorCtorParams() so it will panic in the serializer if that field is null. Switch to using String.nulToEmpty() in the new patch. PS5, Line 222: udfPath > Confusing to omit 'class' from the name. Can you make this udfClass or udfC Done PS5, Line 236: udfPath > while this looks like it's not used in UdfExecutor, this parameter is suppo Pass the jarFile instead. Added a comment about its being "not used". -- To view, visit http://gerrit.cloudera.org:8080/3617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I662e6286dac601ae0e45f18545ef149724aa047e Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
