zabetak commented on code in PR #5312:
URL: https://github.com/apache/hive/pull/5312#discussion_r1679392550
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -688,7 +688,7 @@ Operator genOPTree(ASTNode ast, PlannerContext plannerCtx)
throws SemanticExcept
this.ctx.setCboInfo(cboMsg);
// Determine if we should re-throw the exception OR if we try to
mark the query to retry as non-CBO.
- if (fallbackStrategy.isFatal(e)) {
+ if (fallbackStrategy.isFatal(e) || HiveCalciteUtil.requiresCBO(ast))
{
Review Comment:
The additional disjunctive clause changes a bit the behavior of the fallback
strategies notably when the `ALWAYS/CONSERVATIVE` option is set.
Nevertheless, it makes sense to avoid recompiling a query when we know that
it will fail so the check fits here. Let's just check if we really need a new
method and a tree traversal as I mentioned in the other comment.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveCalciteUtil.java:
##########
@@ -109,6 +110,25 @@ public static boolean
validateASTForUnsupportedTokens(ASTNode ast) {
}
}
+ public static boolean requiresCBO(Tree node) {
+ switch (node.getType()) {
+ // Hive can't support the following features without CBO
+ case HiveParser.TOK_EXCEPTALL:
+ case HiveParser.TOK_EXCEPTDISTINCT:
+ case HiveParser.TOK_INTERSECTALL:
+ case HiveParser.TOK_INTERSECTDISTINCT:
+ case HiveParser.TOK_QUALIFY:
+ return true;
+ default:
+ for (int i = 0; i < node.getChildCount(); i++) {
+ if (requiresCBO(node.getChild(i))) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
Review Comment:
This method traverses the entire tree to find certain tokens something which
was already done before at least once (during parse) or twice (during analyze).
This method seems to incur some unnecessary overhead. There are some other
classes such as `QueryProperties` which could help us avoid the extra
traversals.
Moreover, we already have utility methods such as
`ParseUtils.containsTokenOfType` which can detect if certain tokens exist by
traversing the tree so it seems we don't need the new method.
--
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]
