HIVE-27565: Fix NPE when dropping table in HiveQueryLifeTimeHook::checkAndRollbackCTAS #4549
Conversation
…ckAndRollbackCTAS
| return; | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Not able to check whether the CTAS table directory exists due to: ", e); |
There was a problem hiding this comment.
I am not sure if the error message is reasonable, as some failed queries may not be CTAS type. Welcome to come up with comment here.
There was a problem hiding this comment.
The method calling is private void checkAndRollbackCTAS(QueryLifeTimeHookContext ctx) { and you mean to say this method gets invoked for non CTAS queries as well? If so just remove CTAS from the exception message
There was a problem hiding this comment.
you mean to say this method gets invoked for non CTAS queries as well?
Yse, In my case HIVE-27565 which is Drop type , also invoked checkAndRollbackCTAS(QueryLifeTimeHookContext ctx). But i think this method is only for handling CTAS type queries, so other queries should return early to leave the method.
I will refine the PR to return early if the query is not CTAS.
@ayushtkn It seems not easy to add the test as i only get the short error message instead of the complete error stacktrace in the qltest output, and the short error message can not display I think it is a simple fix, maybe no need to add test? :) |
| @@ -71,20 +71,23 @@ private void checkAndRollbackCTAS(QueryLifeTimeHookContext ctx) { | |||
| QueryPlan queryPlan = ctx.getHookContext().getQueryPlan(); | |||
| boolean isCTAS = Optional.ofNullable(queryPlan.getQueryProperties()) | |||
| .map(queryProps -> queryProps.isCTAS()).orElse(false); | |||
| // return early if the query is not CATS type. | |||
| if (!isCTAS) | |||
There was a problem hiding this comment.
I dont think we must add this since the check is already done below.
|
|
||
| if (isCTAS && tblPath != null) { |
There was a problem hiding this comment.
Please include this. The idea is to clean up any directories which are created by CTAS but are not committed. Hence cleanup must be scheduled only when there is a directory and the query is a CTAS.
| FileSystem fs = tblPath.getFileSystem(conf); | ||
| if (!fs.exists(tblPath)) { | ||
| return; | ||
| if (tblPath != null) { |
There was a problem hiding this comment.
I feel its better to include entire logic below -
if (isCTAS && tblPath != null) {
try {
FileSystem fs = tblPath.getFileSystem(conf);
if (!fs.exists(tblPath)) {
return;
}
} catch (Exception e) {
throw new RuntimeException("Not able to check whether the CTAS table directory exists due to: ", e);
}
// Remaining logic
....
}
The CTAS check and the non-nullability of the path will be checked then in a single if clause.
kgyrtkirk
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
LGTM.
@SourabhBadhya can you check again, if your comments are addressed?
…ckAndRollbackCTAS (apache#4549). (zhangbutao, reviewed by Ayush Saxena, Sourabh Badhya)
What changes were proposed in this pull request?
Why are the changes needed?
In the case of HIVE-27565,
tblPathmaybe null, we can skip this iftblPathis null.hive/ql/src/java/org/apache/hadoop/hive/ql/HiveQueryLifeTimeHook.java
Line 79 in 0447b88
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
How was this patch tested?