You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by gparai <gi...@git.apache.org> on 2018/01/19 22:00:22 UTC

[GitHub] drill pull request #1096: DRILL - 6099 : Push limit past flatten(project) wi...

GitHub user gparai opened a pull request:

    https://github.com/apache/drill/pull/1096

    DRILL - 6099 : Push limit past flatten(project) without pushdown into scan

    @amansinha100 @chunhui-shi can you please review this PR? Thanks!

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/gparai/drill DRILL-6099-master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/drill/pull/1096.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1096
    
----
commit dbd1b22a47de7493d1ead97c699763aad17584f7
Author: Gautam Parai <gp...@...>
Date:   2018-01-18T23:46:42Z

    DRILL - 6099 : Push limit past flatten(project) without pushdown into scan

----


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171711326
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    +      RelOptHelper.some(DrillLimitRel.class, RelOptHelper.any(DrillProjectRel.class)), "DrillPushLimitToScanRule_LimitOnProject") {
         @Override
         public boolean matches(RelOptRuleCall call) {
           DrillLimitRel limitRel = call.rel(0);
    -      DrillScanRel scanRel = call.rel(2);
    -      // For now only applies to Parquet. And pushdown only apply limit but not offset,
    +      DrillProjectRel projectRel = call.rel(1);
    +      // pushdown only apply limit but not offset,
           // so if getFetch() return null no need to run this rule.
    -      if (scanRel.getGroupScan().supportsLimitPushdown() && (limitRel.getFetch() != null)) {
    --- End diff --
    
    Ok, yeah in that case we are not generating a redundant limit.  


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    @chunhui-shi can you please review the new changes (in commit e6dcf14)? Thanks!


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by priteshm <gi...@git.apache.org>.
Github user priteshm commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    @gparai did you get a chance to address @amansinha100 's comment for this PR?


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171086924
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    --- End diff --
    
    There are many instances where we would have a PROJECT on top of the SCAN. The way the rule is refactored now the LIMIT_SCAN rule would not work unless we do LIMIT_PROJECT. Hence, these rules should go together for LIMIT_SCAN to work effectively. That is the reason I kept the rule here rather than creating a new rule.


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171708636
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    +      RelOptHelper.some(DrillLimitRel.class, RelOptHelper.any(DrillProjectRel.class)), "DrillPushLimitToScanRule_LimitOnProject") {
         @Override
         public boolean matches(RelOptRuleCall call) {
           DrillLimitRel limitRel = call.rel(0);
    -      DrillScanRel scanRel = call.rel(2);
    -      // For now only applies to Parquet. And pushdown only apply limit but not offset,
    +      DrillProjectRel projectRel = call.rel(1);
    +      // pushdown only apply limit but not offset,
           // so if getFetch() return null no need to run this rule.
    -      if (scanRel.getGroupScan().supportsLimitPushdown() && (limitRel.getFetch() != null)) {
    --- End diff --
    
    Without a FLATTEN, the LIMIT would be fully pushed past the PROJECT i.e. we would not have a LIMIT on top of the project.


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/drill/pull/1096


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r165788152
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    --- End diff --
    
    I am not sure why this rule is being overloaded for doing limit push past project.  This particular rule is about doing limit pushdown into scan for cases where we have LIMIT-SCAN or LIMIT-PROJECT-SCAN.  I think we should keep this rule as-is but create a separate rule that does a limit push past project.  Was there a strong reason to do it this way ?  Could there be a side effect of removing the check for the Scan ? 


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r163048747
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerPhase.java ---
    @@ -341,7 +346,7 @@ static RuleSet getPruneScanRules(OptimizerRulesContext optimizerRulesContext) {
                 ParquetPruneScanRule.getFilterOnProjectParquet(optimizerRulesContext),
                 ParquetPruneScanRule.getFilterOnScanParquet(optimizerRulesContext),
                 DrillPushLimitToScanRule.LIMIT_ON_SCAN,
    -            DrillPushLimitToScanRule.LIMIT_ON_PROJECT
    +            DrillPushLimitToScanRule.LIMIT_ON_PROJECT_SCAN
    --- End diff --
    
    Not sure if we still need "limit_on_project_scan". In theory, limit_on_project and limit_on_scan should already cover all the cases. Have you tested with "limit_on_project_scan" disabled?


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r164593749
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -121,4 +132,50 @@ protected void doOnMatch(RelOptRuleCall call, DrillLimitRel limitRel, DrillScanR
         }
     
       }
    +
    +  private static boolean isProjectFlatten(RelNode project) {
    --- End diff --
    
    Done.


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171479641
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    +    assert project instanceof Project : "Rel is NOT an instance of project!";
    +    try {
    +      RexVisitor<Void> visitor =
    --- End diff --
    
    Would FLATTEN ever occur within other expressions ?  I believe it always occurs as an independent expression.  If that's the case, it seems to me that having a visitor is overkill.. what do you think ?  Even the original rewrite from project to flatten just iterates over the project exprs here [1].  
    
    [1]  https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/RewriteProjectToFlatten.java#L77


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171085780
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    +      RelOptHelper.some(DrillLimitRel.class, RelOptHelper.any(DrillProjectRel.class)), "DrillPushLimitToScanRule_LimitOnProject") {
         @Override
         public boolean matches(RelOptRuleCall call) {
           DrillLimitRel limitRel = call.rel(0);
    -      DrillScanRel scanRel = call.rel(2);
    -      // For now only applies to Parquet. And pushdown only apply limit but not offset,
    +      DrillProjectRel projectRel = call.rel(1);
    +      // pushdown only apply limit but not offset,
           // so if getFetch() return null no need to run this rule.
    -      if (scanRel.getGroupScan().supportsLimitPushdown() && (limitRel.getFetch() != null)) {
    --- End diff --
    
    We still have the LIMIT_ON_SCAN rule which does that check. This rule is changed from LIMIT_PROJECT_SCAN to LIMIT_PROJECT. The LIMIT_SCAN along with the LIMIT_PROJECT would work as the LIMIT_PROJECT_SCAN.


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171708410
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    --- End diff --
    
    Done


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171480227
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    +      RelOptHelper.some(DrillLimitRel.class, RelOptHelper.any(DrillProjectRel.class)), "DrillPushLimitToScanRule_LimitOnProject") {
         @Override
         public boolean matches(RelOptRuleCall call) {
           DrillLimitRel limitRel = call.rel(0);
    -      DrillScanRel scanRel = call.rel(2);
    -      // For now only applies to Parquet. And pushdown only apply limit but not offset,
    +      DrillProjectRel projectRel = call.rel(1);
    +      // pushdown only apply limit but not offset,
           // so if getFetch() return null no need to run this rule.
    -      if (scanRel.getGroupScan().supportsLimitPushdown() && (limitRel.getFetch() != null)) {
    --- End diff --
    
    One implication of this is suppose the underlying Scan does not support Limit pushdown, you could end up with a plan `Scan->Limit->Project->Limit`  where the Limit above the Scan is redundant (assume that there is no FLATTEN in this query).  Can this be avoided ? 


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171478117
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    +    assert project instanceof Project : "Rel is NOT an instance of project!";
    +    try {
    +      RexVisitor<Void> visitor =
    +          new RexVisitorImpl<Void>(true) {
    +            public Void visitCall(RexCall call) {
    +              if ("flatten".equals(call.getOperator().getName().toLowerCase())) {
    +                throw new Util.FoundOne(call); /* throw exception to interrupt tree walk (this is similar to
    +                                              other utility methods in RexUtil.java */
    +              }
    +              return super.visitCall(call);
    +            }
    +          };
    +      for (RexNode rex : ((Project) project).getProjects()) {
    +        rex.accept(visitor);
    +      }
    +    } catch (Util.FoundOne e) {
    +      Util.swallow(e, null);
    +      return true;
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputSchemaUnknown(RelNode project) {
    --- End diff --
    
    Javadoc


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171708439
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    +    assert project instanceof Project : "Rel is NOT an instance of project!";
    +    try {
    +      RexVisitor<Void> visitor =
    +          new RexVisitorImpl<Void>(true) {
    +            public Void visitCall(RexCall call) {
    +              if ("flatten".equals(call.getOperator().getName().toLowerCase())) {
    +                throw new Util.FoundOne(call); /* throw exception to interrupt tree walk (this is similar to
    +                                              other utility methods in RexUtil.java */
    +              }
    +              return super.visitCall(call);
    +            }
    +          };
    +      for (RexNode rex : ((Project) project).getProjects()) {
    +        rex.accept(visitor);
    +      }
    +    } catch (Util.FoundOne e) {
    +      Util.swallow(e, null);
    +      return true;
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputSchemaUnknown(RelNode project) {
    --- End diff --
    
    Done


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    +1


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171708384
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    +    assert project instanceof Project : "Rel is NOT an instance of project!";
    +    try {
    +      RexVisitor<Void> visitor =
    --- End diff --
    
    Yes, you are correct. If the rewrite does not consider it as embedded within other expressions then it is fine for the utility function to do the same.


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    @amansinha100 I have addressed your review comments. Please take a look. Thanks!


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r164585670
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -121,4 +132,50 @@ protected void doOnMatch(RelOptRuleCall call, DrillLimitRel limitRel, DrillScanR
         }
     
       }
    +
    +  private static boolean isProjectFlatten(RelNode project) {
    --- End diff --
    
    I think it might be more general to name the functions to schemaUnknown(for conert_fromJson), rowCountUnknown(for flatten), so if in future we have some other functions fall in these two categories, we could easily add these functions to the categories. What do you think?


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r171478085
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java ---
    @@ -224,4 +226,64 @@ public Void visitInputRef(RexInputRef inputRef) {
         }
       }
     
    +  public static boolean isLimit0(RexNode fetch) {
    +    if (fetch != null && fetch.isA(SqlKind.LITERAL)) {
    +      RexLiteral l = (RexLiteral) fetch;
    +      switch (l.getTypeName()) {
    +        case BIGINT:
    +        case INTEGER:
    +        case DECIMAL:
    +          if (((long) l.getValue2()) == 0) {
    +            return true;
    +          }
    +      }
    +    }
    +    return false;
    +  }
    +
    +  public static boolean isProjectOutputRowcountUnknown(RelNode project) {
    --- End diff --
    
    Could you add javadoc for this utility function. 


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    Updated version lgtm.  +1


---

[GitHub] drill pull request #1096: DRILL-6099 : Push limit past flatten(project) with...

Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1096#discussion_r165791415
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushLimitToScanRule.java ---
    @@ -55,18 +62,21 @@ public void onMatch(RelOptRuleCall call) {
         }
       };
     
    -  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT =
    -      new DrillPushLimitToScanRule(
    -          RelOptHelper.some(DrillLimitRel.class, RelOptHelper.some(
    -              DrillProjectRel.class, RelOptHelper.any(DrillScanRel.class))),
    -          "DrillPushLimitToScanRule_LimitOnProject") {
    +  public static DrillPushLimitToScanRule LIMIT_ON_PROJECT = new DrillPushLimitToScanRule(
    +      RelOptHelper.some(DrillLimitRel.class, RelOptHelper.any(DrillProjectRel.class)), "DrillPushLimitToScanRule_LimitOnProject") {
         @Override
         public boolean matches(RelOptRuleCall call) {
           DrillLimitRel limitRel = call.rel(0);
    -      DrillScanRel scanRel = call.rel(2);
    -      // For now only applies to Parquet. And pushdown only apply limit but not offset,
    +      DrillProjectRel projectRel = call.rel(1);
    +      // pushdown only apply limit but not offset,
           // so if getFetch() return null no need to run this rule.
    -      if (scanRel.getGroupScan().supportsLimitPushdown() && (limitRel.getFetch() != null)) {
    --- End diff --
    
    I can understand that this check was removed because this matches() method no longer is checking for DrillScanRel, but does that mean that no one is checking the GroupScan for supportsLimitPushdown() ? 


---

[GitHub] drill issue #1096: DRILL-6099 : Push limit past flatten(project) without pus...

Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:

    https://github.com/apache/drill/pull/1096
  
    Once all tests are done, I think it is fine to add 'ready-to-commit' label to the JIRA.


---