Matthew Jacobs 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 PS5, Line 236: for (long ptr: allocations_) { : UnsafeUtil.UNSAFE.freeMemory(ptr); : } : allocations_.clear(); not needed 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 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 worth removing extra spaces for consistency. 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 sense. If you need it to be not be null on l244 you can use the nice function Strings.nullToEmpty(jarFile). Is there any other reason not to keep the old behavior? PS5, Line 222: udfPath Confusing to omit 'class' from the name. Can you make this udfClass or udfClassPath? PS5, Line 236: udfPath while this looks like it's not used in UdfExecutor, this parameter is supposed to be the jar file path. I was confused as to why udfPath was passed twice. Can you just pass in a string like "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
