zabetak commented on a change in pull request #2865:
URL: https://github.com/apache/hive/pull/2865#discussion_r769857181
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
##########
@@ -673,8 +672,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 plan as reAnalyzeAST to retry
- // planning as non-CBO.
+ // Determine if we should re-throw the exception OR if we try to
mark the query to retry as non-CBO.
Review comment:
Interesting, I was about to comment that `ReCompileException` should
better be `CBOException` :) It really feels that this is a better approach.
If we review this part of the code independently of what happens elsewhere
it seems reasonable that we are catching an exception and wrapping it around
another more "high-level" abstraction. This is usually a good coding pattern.
Apart from that, I don't know if it is crucial to retain exactly the same
outputs in tests. For instance, if the changes you mention are due to the
following part of the code:
```
// Wrap all other errors (Should only hit in tests)
throw new SemanticException(e);
```
then it makes believe even more that we shouldn't care.
--
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]
