zabetak commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r928893764
##########
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:
Checking if `msg.isEmpty` is a reasonable fix to avoid the IOBE here. The
code though seems very brittle and relies on various assumptions that are not
specified anywhere.
As an alternative fix I would propose to delete this line altogether and do
the same a few lines above where the same pattern appears.
```suggestion
```
I don't think we need the extra complexity just to remove a space and
semicolon (`; `) from a few messages/explains. I doubt anyone will care. WDYT?
##########
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:
Moreover, I would argue that allowing
`CalcitePlanner.canHandleQbForCbo(queryProperties, conf, true,
needToLogMessage)` to return an empty `String` is not good either but we can
discuss this separately under another JIRA.
--
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]
