[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[asfgit]

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

GitHub user jinfengni opened a pull request:

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

DRILL-1457: Push Limit past through UnionExchange.

Aman, could you please review this pull request?


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

$ git pull https://github.com/jinfengni/incubator-drill DRILL-1457

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

https://github.com/apache/drill/pull/169.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 #169


commit 2cf3601d9bd6ba4b338860d7c4d0c3b009253392
Author: Jinfeng Ni 
Date:   2015-09-22T05:42:23Z

DRILL-1457: Push Limit past through UnionExchange.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[julianhyde]

Github user julianhyde commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40447157
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --

0 is a valid limit. I think you need -1 here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[amansinha100]

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40445053
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestLimitWithExchanges.java
 ---
@@ -26,4 +30,34 @@
   public void testLimitWithExchanges() throws Exception{
 testPhysicalFromFile("limit/limit_exchanges.json");
   }
+
+  @Test
+  public void testPushLimitPastUnionExchange() throws Exception {
+// Push limit past through UnionExchange.
+final String WORKING_PATH = TestTools.getWorkingPath();
+final String TEST_RES_PATH = WORKING_PATH + "/src/test/resources";
+
+try {
+  // case 1. single table query.
+  final String sql = String.format("select * from 
dfs_test.`%s/tpchmulti/region` limit 1 offset 2", TEST_RES_PATH);
+  test("alter session set `planner.slice_target` = 1");
+  // test(sql);
+
+  // Validate the plan
+  final String[] expectedPlan = 
{"(?s)Limit.*UnionExchange.*Limit.*Scan"};
+  final String[] excludedPatterns = {};
+  PlanTestBase.testPlanMatchingPatterns(sql, expectedPlan, 
excludedPatterns);
+
+  // case 2: join query.
+  final String sql2 = String.format("select * from 
dfs_test.`%s/tpchmulti/region` r,  dfs_test.`%s/tpchmulti/nation` n where 
r.r_regionkey = n.n_regionkey limit 1 offset 2", TEST_RES_PATH, TEST_RES_PATH );
+  // Validate the plan
+  final String[] expectedPlan2 = 
{"(?s)Limit.*UnionExchange.*Limit.*Join"};
+  final String[] excludedPatterns2 = {};
+  PlanTestBase.testPlanMatchingPatterns(sql2, expectedPlan2, 
excludedPatterns2);
+} finally {
+  test("alter session set `planner.slice_target` = " + 
ExecConstants.SLICE_TARGET_OPTION.getDefault().getValue());
+}
+// test(sql2);
--- End diff --

Remove extraneous lines..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[amansinha100]

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40444588
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
 ---
@@ -36,11 +36,17 @@
 public abstract class DrillLimitRelBase extends SingleRel implements 
DrillRelNode {
   protected RexNode offset;
   protected RexNode fetch;
+  private boolean pushDown;  // whether limit has been push past its child.
--- End diff --

LIMIT is somewhat unique operator in that even when it is pushed past the 
child, the original LIMIT still remains.  Can you add that in the comments 
because the pushDown flag will be TRUE for the original LIMIT and FALSE for the 
pushed version. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[amansinha100]

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40444664
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
+
+// child Limit uses conservative approach:  use offset 0 and fetch = 
parent limit offset + parent limit fetch.
+final RexNode childFetch = 
limit.getCluster().getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset + 
fetch));
+
+// RexNode newFetch = limit.getCluster().getRexBuilder().makeLiteral()
--- End diff --

Remove this commented line...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[julianhyde]

Github user julianhyde commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40446899
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
 ---
@@ -36,11 +36,17 @@
 public abstract class DrillLimitRelBase extends SingleRel implements 
DrillRelNode {
   protected RexNode offset;
   protected RexNode fetch;
+  private boolean pushDown;  // whether limit has been push past its child.
--- End diff --

Actually sort and aggregate have some of this behavior too. You can do a 
partial sort or partial aggregate in the children but then you need to combine.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[amansinha100]

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40444952
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestLimitWithExchanges.java
 ---
@@ -26,4 +30,34 @@
   public void testLimitWithExchanges() throws Exception{
 testPhysicalFromFile("limit/limit_exchanges.json");
   }
+
+  @Test
+  public void testPushLimitPastUnionExchange() throws Exception {
--- End diff --

I think we should add a simple test with data validation for the LIMIT 
pushdown.  Also include OFFSET + LIMIT.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[hsuanyi]

Github user hsuanyi commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40449942
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --

Overall, the approach seems general enough that it is applicable to more 
than just union-exchange. Can we exploit that ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[julianhyde]

Github user julianhyde commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40465777
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --

@hsuanyi There are several changes going into Calcite for optimizing 
limits. See https://issues.apache.org/jira/browse/CALCITE-831 and re-use those 
rules in drill if applicable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[hsuanyi]

Github user hsuanyi commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40469361
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --

Cool! I read that one. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40484512
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestLimitWithExchanges.java
 ---
@@ -26,4 +30,34 @@
   public void testLimitWithExchanges() throws Exception{
 testPhysicalFromFile("limit/limit_exchanges.json");
   }
+
+  @Test
+  public void testPushLimitPastUnionExchange() throws Exception {
--- End diff --

I added more unit tests with data validation. The data validation is only 
to verify row count, but not the record. That's because depending on which scan 
minor fragment first send the data, the final result could be changing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40484389
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
 ---
@@ -36,11 +36,17 @@
 public abstract class DrillLimitRelBase extends SingleRel implements 
DrillRelNode {
   protected RexNode offset;
   protected RexNode fetch;
+  private boolean pushDown;  // whether limit has been push past its child.
--- End diff --

I add the comment as Aman suggested. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40484523
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestLimitWithExchanges.java
 ---
@@ -26,4 +30,34 @@
   public void testLimitWithExchanges() throws Exception{
 testPhysicalFromFile("limit/limit_exchanges.json");
   }
+
+  @Test
+  public void testPushLimitPastUnionExchange() throws Exception {
+// Push limit past through UnionExchange.
+final String WORKING_PATH = TestTools.getWorkingPath();
+final String TEST_RES_PATH = WORKING_PATH + "/src/test/resources";
+
+try {
+  // case 1. single table query.
+  final String sql = String.format("select * from 
dfs_test.`%s/tpchmulti/region` limit 1 offset 2", TEST_RES_PATH);
+  test("alter session set `planner.slice_target` = 1");
+  // test(sql);
+
+  // Validate the plan
+  final String[] expectedPlan = 
{"(?s)Limit.*UnionExchange.*Limit.*Scan"};
+  final String[] excludedPatterns = {};
+  PlanTestBase.testPlanMatchingPatterns(sql, expectedPlan, 
excludedPatterns);
+
+  // case 2: join query.
+  final String sql2 = String.format("select * from 
dfs_test.`%s/tpchmulti/region` r,  dfs_test.`%s/tpchmulti/nation` n where 
r.r_regionkey = n.n_regionkey limit 1 offset 2", TEST_RES_PATH, TEST_RES_PATH );
+  // Validate the plan
+  final String[] expectedPlan2 = 
{"(?s)Limit.*UnionExchange.*Limit.*Join"};
+  final String[] excludedPatterns2 = {};
+  PlanTestBase.testPlanMatchingPatterns(sql2, expectedPlan2, 
excludedPatterns2);
+} finally {
+  test("alter session set `planner.slice_target` = " + 
ExecConstants.SLICE_TARGET_OPTION.getDefault().getValue());
+}
+// test(sql2);
--- End diff --

Removed that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

Github user jinfengni commented on the pull request:

https://github.com/apache/drill/pull/169#issuecomment-143375295
  
Revise the code based on comments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[jinfengni]

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/169#discussion_r40484865
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
 ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+  public static final RelOptRule INSTANCE = new 
LimitUnionExchangeTransposeRule();
+
+  private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class, 
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel) 
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0, 
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null?  Math.max(0, 
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --

Thanks, Julian!

Good point about the case of fetch == null.  When fetch == null, the query 
should return all the remaining rows starting from offset. In such case, seems 
it does not make sense to push Limit operator, as the underline operator has to 
go through all the records.

I modified the rule, so that it will not push limit when fetch == null. I 
also made change to the row count estimation for the case of fetch == null.

For Sean's comment, this patch is for a special case in Drill's physical 
planing to handle limit on top of UnionExchange. For other regular relational 
operator, as Julian pointed, Calcite-831 will have the optimization rules and 
Drill could re-use them once Calcite-831 is done (Drill-636 was opened for 
those cases).

 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...

[amansinha100]

Github user amansinha100 commented on the pull request:

https://github.com/apache/drill/pull/169#issuecomment-143380757
  
+1  LGTM. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---