kasakrisz commented on code in PR #4762:
URL: https://github.com/apache/hive/pull/4762#discussion_r1345831366
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -623,7 +623,7 @@ Operator genOPTree(ASTNode ast, PlannerContext plannerCtx)
throws SemanticExcept
}
}
Phase1Ctx ctx_1 = initPhase1Ctx();
- if (!doPhase1(newAST, getQB(), ctx_1, null)) {
+ if (!doPhase1(newAST, getQB(), ctx_1, null, null)) {
Review Comment:
how about adding a method overload like
```
doPhase1(ASTNode newAST, QB qb, Phase1Ctx ctx_1, PlannerContext
plannerContext) {
doPhase1(newAST, qb, ctx_1, plannerContext, this.aliasToCTEs)
}
```
This way we can keep the original calls.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -640,22 +640,25 @@ static String genPartValueString(String partColType,
String partVal) {
return returnVal;
}
- private void doPhase1QBExpr(ASTNode ast, QBExpr qbexpr, String id, String
alias, ASTNode tabColNames)
- throws SemanticException {
- doPhase1QBExpr(ast, qbexpr, id, alias, false, tabColNames);
+ private void doPhase1QBExpr(ASTNode ast, QBExpr qbexpr, String id, String
alias, ASTNode tabColNames,
+ Map<String, CTEClause> aliasToCTEs) throws
SemanticException {
+ doPhase1QBExpr(ast, qbexpr, id, alias, false, tabColNames, aliasToCTEs);
}
@SuppressWarnings("nls")
- void doPhase1QBExpr(ASTNode ast, QBExpr qbexpr, String id, String alias,
boolean insideView, ASTNode tabColNames)
- throws SemanticException {
+ void doPhase1QBExpr(ASTNode ast, QBExpr qbexpr, String id, String alias,
boolean insideView, ASTNode tabColNames,
+ Map<String, CTEClause> aliasToCTEs) throws
SemanticException {
assert (ast.getToken() != null);
+ if (aliasToCTEs == null) {
+ aliasToCTEs = this.aliasToCTEs;
+ }
Review Comment:
This may not be necessary if we have the overloaded version of the method I
mentioned above.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -1694,9 +1697,12 @@ private String processLateralView(QB qb, ASTNode
lateralView)
* @throws SemanticException
*/
@SuppressWarnings({"fallthrough", "nls"})
- boolean doPhase1(ASTNode ast, QB qb, Phase1Ctx ctx_1, PlannerContext
plannerCtx)
+ boolean doPhase1(ASTNode ast, QB qb, Phase1Ctx ctx_1, PlannerContext
plannerCtx, Map<String, CTEClause> aliasToCTEs)
throws SemanticException {
+ if (aliasToCTEs == null) {
+ aliasToCTEs = this.aliasToCTEs;
+ }
Review Comment:
This may not be necessary if we have the overloaded version of the method I
mentioned above.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -2161,9 +2167,14 @@ private void getMaterializationMetadata(QB qb) throws
SemanticException {
return;
}
try {
- gatherCTEReferences(qb, rootClause);
+ Map<String, CTEClause> materializationAliasToCTEs = new HashMap<>();
+ for (String key: this.aliasToCTEs.keySet()) {
+ CTEClause clause = this.aliasToCTEs.get(key);
+ materializationAliasToCTEs.put(key, new CTEClause(clause.alias,
clause.cteNode, clause.withColList));
+ }
+ gatherCTEReferences(qb, rootClause, materializationAliasToCTEs);
int threshold = HiveConf.getIntVar(conf,
HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_THRESHOLD);
- for (CTEClause cte : Sets.newHashSet(aliasToCTEs.values())) {
+ for (CTEClause cte :
Sets.newHashSet(materializationAliasToCTEs.values())) {
Review Comment:
I think the existing `CTEClause` objects stored in `this.aliasToCTEs` should
be updated with the ones in `materializationAliasToCTEs`.
Some of the failing test passed when I added
```
this.aliasToCTEs.putAll(materializationAliasToCTEs);
```
because `gatherCTEReferences` collect the number of references of each CTE
in the main query and sibling CTEs. The next few lines sets `cte.materialize`
flag based on the number of references. But these changes are done in the
instances in `materializationAliasToCTEs` only.
Later in `getMetadata` this flag is used to decide whether a CTE should be
prematerialized or not and
`CTEClause` objects are pulled form `this.aliasToCTEs`
https://github.com/apache/hive/blob/de49826286e57976be447c00f9b9abfc41bc305e/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L2280
https://github.com/apache/hive/blob/de49826286e57976be447c00f9b9abfc41bc305e/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L2292
--
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]
