zabetak commented on a change in pull request #1875:
URL: https://github.com/apache/hive/pull/1875#discussion_r565310416
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
##########
@@ -679,27 +680,15 @@ Operator genOPTree(ASTNode ast, PlannerContext
plannerCtx) throws SemanticExcept
// Determine if we should re-throw the exception OR if we try to
mark plan as reAnayzeAST to retry
// planning as non-CBO.
- if (e instanceof CalciteSubquerySemanticException || e instanceof
CalciteViewSemanticException
- || e instanceof CalciteSubqueryRuntimeException) {
- // Non-CBO path for CalciteSubquerySemanticException fails with
completely different error
- // and masks the original failure.
- // Non-CBO path for CalciteViewSemanticException would fail in a
similar way as CBO path.
- throw new SemanticException(e);
- }
-
- boolean isHiveTest = conf.getBoolVar(ConfVars.HIVE_IN_TEST);
- // At this point we retry with CBO off:
- // 1) If this is not test mode (common case)
- // 2) If we are in test mode and we are missing stats
- // 3) if we are in test mode and a CalciteSemanticException is
generated
- reAnalyzeAST = (!isHiveTest || isMissingStats || e instanceof
CalciteSemanticException);
Review comment:
Many thanks for keeping an eye @kgyrtkirk !
In production, (now `CONSERVATIVE` mode) `isHiveTest` is `false` so
`!isHiveTest` becomes `true` thus no matter what happens with `isMissingStats`
and `CalciteSemanticException`, we fallback to legacy optimizer.
In tests, before these changes we fallaback to legacy when the problem is
due to missing stats or CSE. I see this mostly as a convenient way to not
update a big amount of existing tests which otherwise will fail. From my
perspective the fact that CBO fails is not something that should be considered
normal. When writting a test if we know that we cannot handle it with CBO then
I think it would be better to explicitly disable CBO instead of relying on this
fallback mechanism. After these changes, future tests might (depends on the
kind of exception that they come with) fail if they are missing stats.
Now regarding the `RuntimeException`:
- in `ALWAYS` mode we fallback to legacy;
- in `CONSERVATIVE` mode we fallback to legacy (same as before);
- in `NEVER` mode we stop and fail the query;
- in `TEST` mode we stop and fail the query (slightly different than before
where we could retry if `RuntimeException` was paired with a missing stats
problem).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
