You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/07/25 13:43:08 UTC
[GitHub] [hive] zabetak commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
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: 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