impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3756: Fix wrong argument type in HiveStringsTest
Date Fri, 15 Jul 2016 17:36:54 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message