soumyakanti3578 commented on code in PR #5749:
URL: https://github.com/apache/hive/pull/5749#discussion_r2038418049
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -3800,19 +3800,25 @@ private RelNode genGBLogicalPlan(QB qb, RelNode srcRel)
throws SemanticException
groupByOutputRowResolver.setIsExprResolver(true);
if (hasGrpByAstExprs) {
- // 4. Construct GB Keys (ExprNode)
- for (int i = 0; i < groupByNodes.size(); ++i) {
- ASTNode groupByNode = groupByNodes.get(i);
- Map<ASTNode, RexNode> astToRexNodeMap = genAllRexNode(
- groupByNode, groupByInputRowResolver, cluster.getRexBuilder());
- RexNode groupByExpression = astToRexNodeMap.get(groupByNode);
- if (groupByExpression == null) {
- throw new CalciteSemanticException("Invalid Column Reference: "
+ groupByNode.dump(),
- UnsupportedFeature.Invalid_column_reference);
- }
+ try {
+ groupByInputRowResolver.setCheckForAmbiguity(true);
+ // 4. Construct GB Keys (ExprNode)
+ for (int i = 0; i < groupByNodes.size(); ++i) {
+ ASTNode groupByNode = groupByNodes.get(i);
+ Map<ASTNode, RexNode> astToRexNodeMap = genAllRexNode(
+ groupByNode, groupByInputRowResolver,
cluster.getRexBuilder());
+ RexNode groupByExpression = astToRexNodeMap.get(groupByNode);
+ if (groupByExpression == null) {
+ throw new CalciteSemanticException("Invalid Column Reference:
" + groupByNode.dump(),
+ UnsupportedFeature.Invalid_column_reference);
+ }
- addToGBExpr(groupByOutputRowResolver, groupByInputRowResolver,
groupByNode,
- groupByExpression, groupByExpressions, outputColumnNames);
+ addToGBExpr(groupByOutputRowResolver, groupByInputRowResolver,
groupByNode,
+ groupByExpression, groupByExpressions,
outputColumnNames);
+ }
+ }
+ finally {
+ groupByInputRowResolver.setCheckForAmbiguity(false);
}
Review Comment:
I was wondering if we can avoid the extra indentation caused by
`try-finally` here. If we don't use `try-finally` the only concerning thing
that I can think of is that
`groupByInputRowResolver.setCheckForAmbiguity(false);` will not run anymore in
case we throw the `CalciteSemanticException`.
But do we need to set the boolean anyway if we are throwing the Exception?
If not, then it might be better to just remove `try-finally`; then this will
be a small two-line change.
--
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]
