You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "scarlin-cloudera (via GitHub)" <gi...@apache.org> on 2023/03/30 20:27:08 UTC
[GitHub] [hive] scarlin-cloudera commented on a diff in pull request #4132: HIVE-27194: Support expression in limit and offset clauses
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org