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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
