[GitHub] [drill] paul-rogers commented on a change in pull request #2183: DRILL-6547: IllegalStateException: Tried to remove unmanaged buffer in concat function

[GitBox <gi...@apache.org>]

paul-rogers commented on a change in pull request #2183:
URL: https://github.com/apache/drill/pull/2183#discussion_r612022037



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java
##########
@@ -1425,7 +1419,7 @@ public void eval() {
       out.buffer = buffer = buffer.reallocIfNeeded((left.end - left.start) + (right.end - right.start));
       out.start = out.end = 0;
 
-      int id = 0;
+      int id;

Review comment:
       Nit: thanks for removing the unneeded initializer. Might as well move the type into the `for` loop:
   
   ```
   for (int id =...
   ```
   
   Here and below.

##########
File path: exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java
##########
@@ -24,8 +24,10 @@
 import java.util.List;

Review comment:
       I wonder if the tests should move to a file that is a bit more descriptive? Do we have tests for string functions? Else, future folks won't know that the code came from a "bug fix" and to look in this file.
   
   Also, should we have tests for all the functions that were fixed? Presumably those tests don't exist because otherwise we would have found the bugs sooner.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org