scarlin-cloudera commented on code in PR #4132:
URL: https://github.com/apache/hive/pull/4132#discussion_r1153752089
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -3914,33 +3914,24 @@ private RelNode genOBLogicalPlan(QB qb, Pair<RelNode,
RowResolver> selPair,
// 1. OB Expr sanity test
// in strict mode, in the presence of order by, limit must be
// specified
- Integer limit = qb.getParseInfo().getDestLimit(dest);
- if (limit == null) {
+ ASTNode limitExpr = qb.getParseInfo().getDestASTLimit(dest);
+ Integer limitValue = qb.getParseInfo().getDestLimit(dest);
+ if (limitExpr == null && limitValue == null) {
String error = StrictChecks.checkNoLimit(conf);
if (error != null) {
throw new
SemanticException(SemanticAnalyzer.generateErrorMessage(obAST, error));
}
}
OBLogicalPlanGenState obLogicalPlanGenState =
beginGenOBLogicalPlan(obAST, selPair, outermostOB);
-
- // 4. Construct SortRel
RelTraitSet traitSet = cluster.traitSetOf(HiveRelNode.CONVENTION);
RelCollation canonizedCollation = traitSet.canonize(
RelCollationImpl.of(obLogicalPlanGenState.getFieldCollation()));
- RelNode sortRel;
- if (limit != null) {
- Integer offset = qb.getParseInfo().getDestLimitOffset(dest);
- RexNode offsetRN = (offset == null || offset == 0) ?
- null :
cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset));
- RexNode fetchRN =
cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(limit));
- sortRel = new HiveSortLimit(cluster, traitSet,
obLogicalPlanGenState.getObInputRel(), canonizedCollation,
- offsetRN, fetchRN);
- } else {
- sortRel = new HiveSortLimit(cluster, traitSet,
obLogicalPlanGenState.getObInputRel(), canonizedCollation,
- null, null);
- }
+ RelNode sortRel = genLimitLogicalPlan(qb,
obLogicalPlanGenState.getObInputRel(), canonizedCollation);
+ if (sortRel == null) {
Review Comment:
Is it possible to move this "if" check within genLimitLogicalPlan? Seems
more appropriate to have that method responsible for all creations of the
RelNode.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -3914,33 +3914,24 @@ private RelNode genOBLogicalPlan(QB qb, Pair<RelNode,
RowResolver> selPair,
// 1. OB Expr sanity test
// in strict mode, in the presence of order by, limit must be
// specified
- Integer limit = qb.getParseInfo().getDestLimit(dest);
- if (limit == null) {
+ ASTNode limitExpr = qb.getParseInfo().getDestASTLimit(dest);
Review Comment:
Ok, so I think I have a little bit of a small design preference here that I
like better. Take it for what it's worth...
I like having related variables in one class. We now have two variables
representing limit, the limitValue and limitExpr and they both have to be
checked each time. (offset too).
My preference would be to put this in a class like "LimitOffsetExpr" where
it can hold both the expression and value.
Furthermore, we can place the genValueFromConstantExpr method into that
class to retrieve the calculated expression and removes this method from a way
too cluttered up CalcitePlanner class that desperately needs a rewrite (and
prolly call it something different).
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -1880,10 +1880,20 @@ boolean doPhase1(ASTNode ast, QB qb, Phase1Ctx ctx_1,
PlannerContext plannerCtx)
case HiveParser.TOK_LIMIT:
queryProperties.setHasLimit(true);
if (ast.getChildCount() == 2) {
- qbp.setDestLimit(ctx_1.dest,
- Integer.valueOf(ast.getChild(0).getText()),
Integer.valueOf(ast.getChild(1).getText()));
+ if (ast.getChild(1).getChildCount() == 0 &&
ast.getChild(0).getChildCount() == 0) {
Review Comment:
Along with my above suggestion, you can even move some of this code into the
constructor of the new class.
--
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]
