[GitHub] [hive] jfsii commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method

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

jfsii commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r930178880


##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -947,7 +947,7 @@ Pair<Boolean, String> canCBOHandleAst(ASTNode ast, QB qb, PreCboCtx cboCtx) {
     // Now check QB in more detail. canHandleQbForCbo returns null if query can
     // be handled.
     msg = CalcitePlanner.canHandleQbForCbo(queryProperties, conf, true, needToLogMessage);
-    if (msg == null) {
+    if (msg == null || msg.isEmpty()) {
       return Pair.of(true, msg);
     }
     msg = msg.substring(0, msg.length() - 2);

Review Comment:
   I would likely either - move the substring to canHandleQbForCbo, so that method always returns a valid looking message or change canHandleQbForCbo to construct a List<String> of reasons and then use String.join with "; " (and remove the substring code).
   
   I might also even change the method not to bother with taking in verbose and to always create and return error messages. This is on the error path and not some inner loop reading data, so the performance optimization of checking log levels, etc is in the camp of unneeded optimization making the code ugly.
   
   Though I'd go with what @zabetak prefers since they are the primary reviewer.



-- 
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.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org