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/23 02:18:54 UTC
[GitHub] [hive] achennagiri opened a new pull request, #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
achennagiri opened a new pull request, #3474:
URL: https://github.com/apache/hive/pull/3474
### What changes were proposed in this pull request?
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r932172826
##########
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:
The current changes in the PR solve the IOBE, tests are green, and there is no change in behavior so I plan to merge this now.
The rest of the changes proposed by @jfsii are an improvement worth having and are inline with what I briefly mentioned previously:
> allowing `CalcitePlanner.canHandleQbForCbo(queryProperties, conf, true, needToLogMessage)` to return an empty `String` is not good
I would like to get these additional changes merged as well so let's log a new JIRA and I will review ASAP. If nobody takes it in the next few days I can also work on it.
--
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
[GitHub] [hive] achennagiri commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
achennagiri commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r929131513
##########
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:
Yeah, that's right
--
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
[GitHub] [hive] jfsii commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by 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
[GitHub] [hive] achennagiri commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
achennagiri commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r929131117
##########
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:
Oh okay. That makes sense. We could keep the space and semicolon. I wasn't sure why that was added. I looked around for other places if we did something similar and I couldn't find. So, we are consistent also that way.
--
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
[GitHub] [hive] achennagiri commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
achennagiri commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r929335839
##########
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:
Also, some of the explain analyze queries would show something like this
`Plan not optimized by CBO because the statement has PTF; ` as opposed to
`Plan not optimized by CBO because the statement has PTF
`
which should be fine I guess
--
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
[GitHub] [hive] achennagiri commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
achennagiri commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r932428871
##########
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:
Great. Thanks for the review Stamatis!
--
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
[GitHub] [hive] achennagiri commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
achennagiri commented on code in PR #3474:
URL: https://github.com/apache/hive/pull/3474#discussion_r929337365
##########
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:
This will need some changes to the query output files as well. Also, since it is affecting the aesthetics for the user directly, I am now thinking whethe we should really remove the below line
`msg = msg.substring(0, msg.length() - 2);`. What do you say?
--
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
[GitHub] [hive] zabetak commented on a diff in pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [hive] nrg4878 commented on pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on PR #3474:
URL: https://github.com/apache/hive/pull/3474#issuecomment-1193241476
@zabetak I am not familiar with the CBO code as much. Could you please review this simple change? Thank you
--
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
[GitHub] [hive] zabetak closed pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #3474: HIVE-26426: Avoid StringIndexOutOfBoundsException in canCBOHandleAst() method
URL: https://github.com/apache/hive/pull/3474
--
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