zabetak commented on code in PR #5294:
URL: https://github.com/apache/hive/pull/5294#discussion_r1679080458
##########
itests/hive-blobstore/src/test/queries/clientpositive/having.q:
##########
@@ -1,4 +1,5 @@
-- Test HAVING clause
Review Comment:
Adding more tests and EXPLAIN CBO plans is a good idea.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -5167,7 +5167,14 @@ private RelNode genGBHavingLogicalPlan(QB qb, RelNode
srcRel) throws SemanticExc
UnsupportedFeature.Having_clause_without_any_groupby);
}
ASTNode targetNode = (ASTNode) havingClause.getChild(0);
- validateNoHavingReferenceToAlias(qb, targetNode);
+ QBParseInfo qbPI = qb.getParseInfo();
+ Map<ASTNode, String> exprToAlias = qbPI.getAllExprToColumnAlias();
+ RowResolver inputRR = relToHiveRR.get(srcRel);
+ for (ASTNode astNode : exprToAlias.keySet()) {
+ if (inputRR.getExpression(astNode) != null) {
+ inputRR.put("", exprToAlias.get(astNode),
inputRR.getExpression(astNode));
+ }
+ }
Review Comment:
This part is basically modifying the `RowResolver` adding some kind of
reverse mappings. Moreover, this logic is duplicated in
`SemanticAnalyzer#genHavingPlan`. I think it makes sense to refactor this
snippet and create a new method in the `RowResolver` class with a proper name
(and javadoc if necessary) that clarifies what is the code supposed to do. The
only parameter to the method should be a `Map<ASTNode, String>`.
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
