You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "jfsii (via GitHub)" <gi...@apache.org> on 2023/03/30 16:32:16 UTC
[GitHub] [hive] jfsii commented on a diff in pull request #4132: HIVE-27194: Support expression in limit and offset clauses
jfsii commented on code in PR #4132:
URL: https://github.com/apache/hive/pull/4132#discussion_r1153499754
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -4179,30 +4170,53 @@ public RelNode endGenOBLogicalPlan(OBLogicalPlanGenState obLogicalPlanGenState,
}
}
- private RelNode genLimitLogicalPlan(QB qb, RelNode srcRel) throws SemanticException {
- HiveRelNode sortRel = null;
- QBParseInfo qbp = getQBParseInfo(qb);
- SimpleEntry<Integer,Integer> entry =
- qbp.getDestToLimit().get(qbp.getClauseNames().iterator().next());
- Integer offset = (entry == null) ? null : entry.getKey();
- Integer fetch = (entry == null) ? null : entry.getValue();
-
- if (fetch != null) {
- RexNode offsetRN = (offset == null || offset == 0) ?
- null : cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset));
- RexNode fetchRN = cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(fetch));
- RelTraitSet traitSet = cluster.traitSetOf(HiveRelNode.CONVENTION);
- RelCollation canonizedCollation = traitSet.canonize(RelCollations.EMPTY);
- sortRel = new HiveSortLimit(cluster, traitSet, srcRel, canonizedCollation, offsetRN, fetchRN);
-
- RowResolver inputRR = relToHiveRR.get(srcRel);
- RowResolver outputRR = inputRR.duplicate();
- ImmutableMap<String, Integer> hiveColNameCalcitePosMap = buildHiveToCalciteColumnMap(outputRR);
- relToHiveRR.put(sortRel, outputRR);
- relToHiveColNameCalcitePosMap.put(sortRel, hiveColNameCalcitePosMap);
+ /**
+ * Generates values from constant expression node.
+ * @param inputRR Row resolver
+ * @param expr AST Node which is a constant expression
+ * @return Object value of the expression
+ * @throws SemanticException
+ */
+ private Object genValueFromConstantExpr(RowResolver inputRR, ASTNode expr) throws SemanticException {
+ ExprNodeDesc exprNode = genExprNodeDesc(expr, inputRR, true, true);
+ if (exprNode instanceof ExprNodeConstantDesc) {
+ ExprNodeConstantDesc offsetConstantExprNode = (ExprNodeConstantDesc) exprNode;
+ return offsetConstantExprNode.getValue();
+ } else {
+ throw new SemanticException("Only constant expressions are supported");
}
+ }
- return sortRel;
+ private RelNode genLimitLogicalPlan(QB qb, RelNode srcRel, RelCollation canonizedCollation) throws SemanticException {
+ QBParseInfo qbp = getQBParseInfo(qb);
+ RowResolver inputRR = relToHiveRR.get(srcRel);
+ // Fetch the limit value from the query, or generate it from the limit expression if it exists
+ Integer limitValue = qbp.getDestLimit(qbp.getClauseNames().iterator().next());
Review Comment:
Maybe store qbp.getClauseNames().iterator().next() in a local variable and reuse - it might make the code a tiny more readable.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -4179,30 +4170,53 @@ public RelNode endGenOBLogicalPlan(OBLogicalPlanGenState obLogicalPlanGenState,
}
}
- private RelNode genLimitLogicalPlan(QB qb, RelNode srcRel) throws SemanticException {
- HiveRelNode sortRel = null;
- QBParseInfo qbp = getQBParseInfo(qb);
- SimpleEntry<Integer,Integer> entry =
- qbp.getDestToLimit().get(qbp.getClauseNames().iterator().next());
- Integer offset = (entry == null) ? null : entry.getKey();
- Integer fetch = (entry == null) ? null : entry.getValue();
-
- if (fetch != null) {
- RexNode offsetRN = (offset == null || offset == 0) ?
- null : cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset));
- RexNode fetchRN = cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(fetch));
- RelTraitSet traitSet = cluster.traitSetOf(HiveRelNode.CONVENTION);
- RelCollation canonizedCollation = traitSet.canonize(RelCollations.EMPTY);
- sortRel = new HiveSortLimit(cluster, traitSet, srcRel, canonizedCollation, offsetRN, fetchRN);
-
- RowResolver inputRR = relToHiveRR.get(srcRel);
- RowResolver outputRR = inputRR.duplicate();
- ImmutableMap<String, Integer> hiveColNameCalcitePosMap = buildHiveToCalciteColumnMap(outputRR);
- relToHiveRR.put(sortRel, outputRR);
- relToHiveColNameCalcitePosMap.put(sortRel, hiveColNameCalcitePosMap);
+ /**
+ * Generates values from constant expression node.
+ * @param inputRR Row resolver
+ * @param expr AST Node which is a constant expression
+ * @return Object value of the expression
+ * @throws SemanticException
+ */
+ private Object genValueFromConstantExpr(RowResolver inputRR, ASTNode expr) throws SemanticException {
+ ExprNodeDesc exprNode = genExprNodeDesc(expr, inputRR, true, true);
+ if (exprNode instanceof ExprNodeConstantDesc) {
+ ExprNodeConstantDesc offsetConstantExprNode = (ExprNodeConstantDesc) exprNode;
+ return offsetConstantExprNode.getValue();
+ } else {
+ throw new SemanticException("Only constant expressions are supported");
Review Comment:
Might be nice if you specify where constant expressions are expected - I.E. OFFSET LIMIT?
It would help the user determine where to look for the error in the query.
Are you able to write a query to trigger this exception? If so, I would consider adding a negative test.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -4179,30 +4170,53 @@ public RelNode endGenOBLogicalPlan(OBLogicalPlanGenState obLogicalPlanGenState,
}
}
- private RelNode genLimitLogicalPlan(QB qb, RelNode srcRel) throws SemanticException {
- HiveRelNode sortRel = null;
- QBParseInfo qbp = getQBParseInfo(qb);
- SimpleEntry<Integer,Integer> entry =
- qbp.getDestToLimit().get(qbp.getClauseNames().iterator().next());
- Integer offset = (entry == null) ? null : entry.getKey();
- Integer fetch = (entry == null) ? null : entry.getValue();
-
- if (fetch != null) {
- RexNode offsetRN = (offset == null || offset == 0) ?
- null : cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset));
- RexNode fetchRN = cluster.getRexBuilder().makeExactLiteral(BigDecimal.valueOf(fetch));
- RelTraitSet traitSet = cluster.traitSetOf(HiveRelNode.CONVENTION);
- RelCollation canonizedCollation = traitSet.canonize(RelCollations.EMPTY);
- sortRel = new HiveSortLimit(cluster, traitSet, srcRel, canonizedCollation, offsetRN, fetchRN);
-
- RowResolver inputRR = relToHiveRR.get(srcRel);
- RowResolver outputRR = inputRR.duplicate();
- ImmutableMap<String, Integer> hiveColNameCalcitePosMap = buildHiveToCalciteColumnMap(outputRR);
- relToHiveRR.put(sortRel, outputRR);
- relToHiveColNameCalcitePosMap.put(sortRel, hiveColNameCalcitePosMap);
+ /**
+ * Generates values from constant expression node.
+ * @param inputRR Row resolver
+ * @param expr AST Node which is a constant expression
+ * @return Object value of the expression
+ * @throws SemanticException
+ */
+ private Object genValueFromConstantExpr(RowResolver inputRR, ASTNode expr) throws SemanticException {
+ ExprNodeDesc exprNode = genExprNodeDesc(expr, inputRR, true, true);
+ if (exprNode instanceof ExprNodeConstantDesc) {
+ ExprNodeConstantDesc offsetConstantExprNode = (ExprNodeConstantDesc) exprNode;
+ return offsetConstantExprNode.getValue();
+ } else {
+ throw new SemanticException("Only constant expressions are supported");
}
+ }
- return sortRel;
+ private RelNode genLimitLogicalPlan(QB qb, RelNode srcRel, RelCollation canonizedCollation) throws SemanticException {
+ QBParseInfo qbp = getQBParseInfo(qb);
+ RowResolver inputRR = relToHiveRR.get(srcRel);
+ // Fetch the limit value from the query, or generate it from the limit expression if it exists
+ Integer limitValue = qbp.getDestLimit(qbp.getClauseNames().iterator().next());
+ ASTNode limitExpr = qbp.getDestASTLimit(qbp.getClauseNames().iterator().next());
+ if (limitValue == null && limitExpr != null) {
+ limitValue = (Integer) genValueFromConstantExpr(inputRR, limitExpr);
+ }
+ // Fetch the offset value from the query, or generate it from the offset expression if it exists
+ Integer offsetValue = qbp.getDestLimitOffset(qbp.getClauseNames().iterator().next());
+ ASTNode offsetExpr = qbp.getDestASTOffset(qbp.getClauseNames().iterator().next());
+ if (offsetValue == null && offsetExpr != null) {
+ offsetValue = (Integer) genValueFromConstantExpr(inputRR, offsetExpr);
+ }
+ if (limitValue == null) {
Review Comment:
This likely should moved to just after limitValue is set
--
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