You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2022/04/05 01:40:32 UTC

[GitHub] [calcite] JiajunBernoulli opened a new pull request, #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

JiajunBernoulli opened a new pull request, #2763:
URL: https://github.com/apache/calcite/pull/2763

   I searched the code of `RexLiteral#intValue` in core module. Found that four class failed when LIMIT/OFFSET parameter is Dynamic. 
   - RelMdMaxRowCount
   - RelMdMinRowCount
   - RelMdUtil
   - RelDecorrelator
   I fixed them, and added corresponding unit tests.


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r846584632


##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -476,14 +477,29 @@ protected RexNode removeCorrelationExpr(
 
     RelCollation oldCollation = rel.getCollation();
     RelCollation newCollation = RexUtil.apply(mapping, oldCollation);
-
-    final int offset = rel.offset == null ? -1 : RexLiteral.intValue(rel.offset);
-    final int fetch = rel.fetch == null ? -1 : RexLiteral.intValue(rel.fetch);
+    int offset = -1;

Review Comment:
   Good advice.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] JiajunBernoulli commented on pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on PR #2763:
URL: https://github.com/apache/calcite/pull/2763#issuecomment-1098152734

   > @JiajunBernoulli I left you a minor comment in a test. Apart from that, I think the PR is in a good shape and could be merged soon (after squashing commits into a single one, I'd suggest to keep the Jira title as message for the single commit after squash).
   Thanks for your review, I used EMP_SIZE to replace 14D in single commit.


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r847850008


##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -3196,12 +3197,40 @@ public RelBuilder sort(RelCollation collation) {
    */
   public RelBuilder sortLimit(int offset, int fetch,
       Iterable<? extends RexNode> nodes) {
+    final @Nullable RexNode offsetNode = offset <= 0 ? null : literal(offset);
+    final @Nullable RexNode fetchNode = fetch < 0 ? null : literal(fetch);
+    if (offsetNode == null && fetch == 0 && config.simplifyLimit()) {

Review Comment:
   Never mind, I deleted the duplicate code. Thank you again~



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r844815785


##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -476,14 +477,29 @@ protected RexNode removeCorrelationExpr(
 
     RelCollation oldCollation = rel.getCollation();
     RelCollation newCollation = RexUtil.apply(mapping, oldCollation);
-
-    final int offset = rel.offset == null ? -1 : RexLiteral.intValue(rel.offset);
-    final int fetch = rel.fetch == null ? -1 : RexLiteral.intValue(rel.fetch);
+    int offset = -1;

Review Comment:
   See my comment in RelBuilder, which could help simplify this code.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r844815347


##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -3196,12 +3197,23 @@ public RelBuilder sort(RelCollation collation) {
    */
   public RelBuilder sortLimit(int offset, int fetch,
       Iterable<? extends RexNode> nodes) {
+    return sortLimit(offset, fetch, nodes, null, null);
+  }
+
+  public RelBuilder sortLimit(int offset, int fetch,

Review Comment:
   Personally, I find this API confusing, because it could lead to situations where e.g. a valid offset and a valid dynamicOffset are passed at the same time.
   What about keeping the "old" sortLimit method and adding a new one with RexNode parameters (with the corresponding check to verify that they are literals or parameters or null), i.e. something like:
   ```
     /** Creates a {@link Sort} ... */
     public RelBuilder sortLimit(int offset, int fetch,
         Iterable<? extends RexNode> nodes) {
       final @Nullable RexNode offsetNode = offset <= 0 ? null : literal(offset);
       final @Nullable RexNode fetchNode = fetch < 0 ? null : literal(fetch);
       if (offsetNode == null && fetch == 0 && config.simplifyLimit()) {
         return empty();
       }
       return sortLimit(offsetNode, fetchNode, nodes);
     }
   
     /**
      * TODO javadoc
      */
     public RelBuilder sortLimit(@Nullable RexNode offsetNode, @Nullable RexNode fetchNode,
         Iterable<? extends RexNode> nodes) {
       if (offsetNode != null) {
         if (!(offsetNode instanceof RexLiteral || offsetNode instanceof RexDynamicParam)) {
           throw new IllegalArgumentException("...");
         }
       }
       if (fetchNode != null) {
         if (!(fetchNode instanceof RexLiteral || fetchNode instanceof RexDynamicParam)) {
           throw new IllegalArgumentException("...");
         }
       }
   
       final Registrar registrar = new Registrar(fields(), ImmutableList.of());
       ...
   ```
   I think this approach would also simplify the new code in `RelDecorrelator`, where this new method could be used like: `sortLimit(rel.offset, rel.fetch, relBuilder.fields(newCollation))`
   WDYT?



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r849220180


##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -644,6 +644,15 @@ final RelMetadataFixture sql(String sql) {
     fixture.assertThatRowCount(is(9D), is(0D), is(10d));
   }
 
+  @Test void testRowCountSortLimitOffsetDynamic() {
+    sql("select * from emp order by ename limit ? offset ?")
+        .assertThatRowCount(is(14D), is(0D), is(Double.POSITIVE_INFINITY));

Review Comment:
   minor: in these tests, instead of `is(14D)`, I think it would be a bit better to use `is(EMP_SIZE)` (as some other tests in the same class do).
   Similarly, instead of `is(13D)` we could use `is(EMP_SIZE - 1)`.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada commented on PR #2763:
URL: https://github.com/apache/calcite/pull/2763#issuecomment-1097712293

   @JiajunBernoulli I left you a minor comment in a test. Apart from that, I think the PR is in a good shape and could be merged soon (after squashing commits into a single one, I'd suggest to keep the Jira title as message for the single commit after squash).


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r846584549


##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -3196,12 +3197,23 @@ public RelBuilder sort(RelCollation collation) {
    */
   public RelBuilder sortLimit(int offset, int fetch,
       Iterable<? extends RexNode> nodes) {
+    return sortLimit(offset, fetch, nodes, null, null);
+  }
+
+  public RelBuilder sortLimit(int offset, int fetch,

Review Comment:
   Thank you for your advice, I have finished the refactor. 



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on a diff in pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada commented on code in PR #2763:
URL: https://github.com/apache/calcite/pull/2763#discussion_r847065258


##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -3196,12 +3197,40 @@ public RelBuilder sort(RelCollation collation) {
    */
   public RelBuilder sortLimit(int offset, int fetch,
       Iterable<? extends RexNode> nodes) {
+    final @Nullable RexNode offsetNode = offset <= 0 ? null : literal(offset);
+    final @Nullable RexNode fetchNode = fetch < 0 ? null : literal(fetch);
+    if (offsetNode == null && fetch == 0 && config.simplifyLimit()) {

Review Comment:
   Sorry, I know I was the one that proposed this code, but I think we should not include this `if (...) return empty()` simplification, because it will be a duplication (this same simplification will be part any way of the overloaded method with RexNode parameters, so I think we should remove it from this one).



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada merged pull request #2763: [CALCITE-5048] Optimizer support dynamic parameter in LIMIT/OFFSET

Posted by GitBox <gi...@apache.org>.
rubenada merged PR #2763:
URL: https://github.com/apache/calcite/pull/2763


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org