[GitHub] [hive] zabetak commented on a diff in pull request #4132: HIVE-27194: Support expression in limit and offset clauses

["zabetak (via GitHub)" <gi...@apache.org>]

zabetak commented on code in PR #4132:
URL: https://github.com/apache/hive/pull/4132#discussion_r1159878060


##########
ql/src/test/queries/clientpositive/limit_expression.q:
##########
@@ -0,0 +1,11 @@
+--! qt:dataset:src
+-- Limit and offset queries
+select key from src limit 1;
+select key from src limit 1 offset 1;
+select key from src limit (1+2*3);
+select key from src limit (1+2*3) offset (3*4*5);

Review Comment:
   Please add the following tests:
   ```
   select key from src limit (1) offset (1);
   select key from src limit 1 offset (1+2);
   select key from src limit (1+2) offset 1;
   ```



##########
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());

Review Comment:
   It seems that getDestToLimit method becomes unused after these changes; we should remove it plus any code related to that.



##########
ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java:
##########
@@ -498,6 +504,21 @@ public void setExprToColumnAlias(ASTNode expr, String alias) {
     exprToColumnAlias.put(expr,  StringInternUtils.internIfNotNull(alias));
   }
 
+  public void setDestASTLimit(String dest, ASTNode limitExpr) {
+    destToASTLimit.put(dest, limitExpr);
+  }
+
+  public ASTNode getDestASTLimit(String dest) {
+    return destToASTLimit.get(dest);
+  }
+  public void setDestASTOffset(String dest, ASTNode offsetExpr) {
+    destToASTOffset.put(dest, offsetExpr);
+  }
+
+  public ASTNode getDestASTOffset(String dest) {
+    return destToASTOffset.get(dest);
+  }
+

Review Comment:
   Instead of adding new methods and structures we could possibly make the existing ones more general; i.e., 
   ```
   private final Map<String, SimpleEntry<Integer, Integer>> destToLimit;
   private final Map<String, SimpleEntry<ASTNode, ASTNode>> destToLimit;
   ```



##########
ql/src/test/queries/clientpositive/limit_expression.q:
##########
@@ -0,0 +1,11 @@
+--! qt:dataset:src
+-- Limit and offset queries
+select key from src limit 1;
+select key from src limit 1 offset 1;
+select key from src limit (1+2*3);
+select key from src limit (1+2*3) offset (3*4*5);
+select key from src order by key limit (1+2*3) offset (3*4*5);
+
+-- Nested queries
+select key from (select * from src limit (1+2*3)) q1;
+select key from (select * from src limit (1+2*3) offset (3*4*5)) q1;

Review Comment:
   nit: add new line



##########
ql/src/test/queries/clientpositive/limit_expression.q:
##########
@@ -0,0 +1,11 @@
+--! qt:dataset:src
+-- Limit and offset queries
+select key from src limit 1;
+select key from src limit 1 offset 1;
+select key from src limit (1+2*3);
+select key from src limit (1+2*3) offset (3*4*5);

Review Comment:
   How about the following tests:
   * very large numeric literals;
   * arithmetic operations that may cause int/long overflow/underflow;
   * arithmetic operations with non integer numeric types;
   * tests with `hive.cbo.enable=false`;
   
   Should we add them; do we need them?



##########
ql/src/test/queries/clientpositive/limit_expression.q:
##########
@@ -0,0 +1,11 @@
+--! qt:dataset:src

Review Comment:
   Please add also positive and negative unit tests for the AST/Parser (e.g., `TestValuesClause`).



##########
parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g:
##########
@@ -2789,12 +2789,14 @@ destination
    | KW_TABLE tableOrPartition -> tableOrPartition
    ;
 
+
 limitClause
 @init { pushMsg("limit clause", state); }
 @after { popMsg(state); }
    :
    KW_LIMIT ((offset=Number COMMA)? num=Number) -> ^(TOK_LIMIT ($offset)? $num)
    | KW_LIMIT num=Number KW_OFFSET offset=Number -> ^(TOK_LIMIT ($offset)? $num)
+   | KW_LIMIT LPAREN numExpr=expression RPAREN (KW_OFFSET LPAREN offsetExpr=expression RPAREN)? -> ^(TOK_LIMIT ($offsetExpr)? $numExpr)

Review Comment:
   The commit message/PR summary/JIRA summary imply that we add support for arbitrary expressions. The parsing code here also seems to be general enough to handle those. In other places though we seem to expect only INT arithmetic.
   
   Please revise the code or summary as needed to reflect better the intention of the change.



##########
parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g:
##########
@@ -2789,12 +2789,14 @@ destination
    | KW_TABLE tableOrPartition -> tableOrPartition
    ;
 
+

Review Comment:
   nit: remove empty line



##########
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:
   I agree with Steve's comment. Moreover, the `limitValue` is a special case of `limitExpr` so I was thinking that it should be possible to keep only the latter; same for offset.



##########
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 {

Review Comment:
   Move the new method after `genLimitLogicalPlan` to make new changes in `genLimitLogicalPlan` more apparent and easier to review.



-- 
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