You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by dz...@apache.org on 2022/02/13 06:11:59 UTC
[drill] branch master updated: DRILL-8131: Infinite planning when JDBC or Phoenix plugin is enabled (#2459)
This is an automated email from the ASF dual-hosted git repository.
dzamo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/drill.git
The following commit(s) were added to refs/heads/master by this push:
new cd21d31 DRILL-8131: Infinite planning when JDBC or Phoenix plugin is enabled (#2459)
cd21d31 is described below
commit cd21d316cbf0e2da77e2f9b59b19fecdffd0e36b
Author: Volodymyr Vysotskyi <vv...@gmail.com>
AuthorDate: Sun Feb 13 08:11:49 2022 +0200
DRILL-8131: Infinite planning when JDBC or Phoenix plugin is enabled (#2459)
---
contrib/storage-jdbc/pom.xml | 6 ++++
.../drill/exec/store/jdbc/DrillJdbcConvention.java | 4 +--
.../drill/exec/store/jdbc/rules/JdbcLimitRule.java | 35 ++++++++++++++++------
.../exec/store/jdbc/TestJdbcPluginWithMySQLIT.java | 12 ++++++++
.../store/phoenix/rules/PhoenixConvention.java | 4 +--
.../exec/planner/common/DrillLimitRelBase.java | 14 +++++----
.../drill/exec/planner/logical/DrillLimitRel.java | 5 ++++
.../drill/exec/planner/physical/LimitPrel.java | 5 ++++
.../store/enumerable/plan/DrillJdbcRuleBase.java | 6 ++++
.../drill/exec/store/plan/rel/PluginLimitRel.java | 11 +++++--
.../exec/store/plan/rule/PluginLimitRule.java | 10 ++++++-
11 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/contrib/storage-jdbc/pom.xml b/contrib/storage-jdbc/pom.xml
old mode 100755
new mode 100644
index de78021..534b44a
--- a/contrib/storage-jdbc/pom.xml
+++ b/contrib/storage-jdbc/pom.xml
@@ -68,6 +68,12 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.apache.drill.contrib.data</groupId>
+ <artifactId>tpch-sample-data</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>${mysql.connector.version}</version>
diff --git a/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/DrillJdbcConvention.java b/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/DrillJdbcConvention.java
index c469502..55ac145 100644
--- a/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/DrillJdbcConvention.java
+++ b/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/DrillJdbcConvention.java
@@ -36,7 +36,6 @@ import org.apache.calcite.sql.SqlDialect;
import org.apache.drill.exec.planner.RuleInstance;
import org.apache.drill.exec.planner.logical.DrillRel;
import org.apache.drill.exec.planner.logical.DrillRelFactories;
-import org.apache.drill.exec.planner.physical.Prel;
import org.apache.drill.exec.store.enumerable.plan.DrillJdbcRuleBase;
import org.apache.drill.exec.store.enumerable.plan.VertexDrelConverterRule;
import org.apache.drill.exec.store.jdbc.rules.JdbcLimitRule;
@@ -66,8 +65,7 @@ public class DrillJdbcConvention extends JdbcConvention {
List<RelTrait> inputTraits = Arrays.asList(
Convention.NONE,
- DrillRel.DRILL_LOGICAL,
- Prel.DRILL_PHYSICAL);
+ DrillRel.DRILL_LOGICAL);
ImmutableSet.Builder<RelOptRule> builder = ImmutableSet.<RelOptRule>builder()
.addAll(calciteJdbcRules)
diff --git a/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java b/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java
index dc44c72..6bab89a 100644
--- a/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java
+++ b/contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java
@@ -17,14 +17,20 @@
*/
package org.apache.drill.exec.store.jdbc.rules;
-import org.apache.calcite.plan.RelOptRuleCall;
import org.apache.calcite.plan.RelTrait;
import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
import org.apache.calcite.sql.dialect.MssqlSqlDialect;
import org.apache.drill.exec.planner.common.DrillLimitRelBase;
import org.apache.drill.exec.store.enumerable.plan.DrillJdbcRuleBase;
+import org.apache.drill.exec.store.enumerable.plan.DrillJdbcSort;
import org.apache.drill.exec.store.jdbc.DrillJdbcConvention;
+import java.math.BigDecimal;
+import java.util.Collections;
+
public class JdbcLimitRule extends DrillJdbcRuleBase.DrillJdbcLimitRule {
private final DrillJdbcConvention convention;
@@ -34,17 +40,28 @@ public class JdbcLimitRule extends DrillJdbcRuleBase.DrillJdbcLimitRule {
}
@Override
- public boolean matches(RelOptRuleCall call) {
- DrillLimitRelBase limit = call.rel(0);
- if (super.matches(call)) {
+ public RelNode convert(RelNode rel) {
+ DrillLimitRelBase limit = (DrillLimitRelBase) rel;
+ if (limit.getOffset() == null
+ || !limit.getTraitSet().contains(RelCollations.EMPTY)
+ || !(convention.getPlugin().getDialect() instanceof MssqlSqlDialect)) {
+ return super.convert(limit);
+ } else {
// MS SQL doesn't support either OFFSET or FETCH without ORDER BY.
// But for the case of FETCH without OFFSET, Calcite generates TOP N
// instead of FETCH, and it is supported by MS SQL.
- // So do not push down only the limit with both OFFSET and FETCH but without ORDER BY.
- return limit.getOffset() == null
- || !limit.getTraitSet().contains(RelCollations.EMPTY)
- || !(convention.getPlugin().getDialect() instanceof MssqlSqlDialect);
+ // So do splitting the limit with both OFFSET and FETCH but without ORDER BY
+ int offset = Math.max(0, RexLiteral.intValue(limit.getOffset()));
+ int fetch = Math.max(0, RexLiteral.intValue(limit.getFetch()));
+
+ // child Limit uses conservative approach: use offset 0 and fetch = parent limit offset + parent limit fetch.
+ RexNode childFetch = limit.getCluster().getRexBuilder().makeExactLiteral(BigDecimal.valueOf(offset + fetch));
+
+ RelNode jdbcSort = new DrillJdbcSort(limit.getCluster(), limit.getTraitSet().plus(RelCollations.EMPTY).replace(this.out).simplify(),
+ convert(limit.getInput(), limit.getInput().getTraitSet().replace(this.out).simplify()),
+ RelCollations.EMPTY, null, childFetch);
+
+ return limit.copy(limit.getTraitSet(), Collections.singletonList(jdbcSort), true);
}
- return false;
}
}
diff --git a/contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java b/contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java
index a4f010c..d735473 100644
--- a/contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java
+++ b/contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java
@@ -395,4 +395,16 @@ public class TestJdbcPluginWithMySQLIT extends ClusterTest {
.exclude("Limit\\(")
.match();
}
+
+ @Test // DRILL-8131
+ public void testParquetLimitWithSort() throws Exception {
+ queryBuilder()
+ .sql("SELECT n_name\n" +
+ "FROM cp.`/tpch/nation.parquet`\n" +
+ "ORDER BY n_name DESC\n" +
+ "LIMIT 1")
+ .planMatcher()
+ .include("Limit\\(")
+ .match();
+ }
}
diff --git a/contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/rules/PhoenixConvention.java b/contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/rules/PhoenixConvention.java
index 6d4ff48..48ddaad 100644
--- a/contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/rules/PhoenixConvention.java
+++ b/contrib/storage-phoenix/src/main/java/org/apache/drill/exec/store/phoenix/rules/PhoenixConvention.java
@@ -38,7 +38,6 @@ import org.apache.calcite.sql.SqlDialect;
import org.apache.drill.exec.planner.RuleInstance;
import org.apache.drill.exec.planner.logical.DrillRel;
import org.apache.drill.exec.planner.logical.DrillRelFactories;
-import org.apache.drill.exec.planner.physical.Prel;
import org.apache.drill.exec.store.enumerable.plan.DrillJdbcRuleBase;
import org.apache.drill.exec.store.enumerable.plan.VertexDrelConverterRule;
import org.apache.drill.exec.store.phoenix.PhoenixStoragePlugin;
@@ -66,8 +65,7 @@ public class PhoenixConvention extends JdbcConvention {
List<RelTrait> inputTraits = Arrays.asList(
Convention.NONE,
- DrillRel.DRILL_LOGICAL,
- Prel.DRILL_PHYSICAL);
+ DrillRel.DRILL_LOGICAL);
ImmutableSet.Builder<RelOptRule> builder = ImmutableSet.<RelOptRule>builder()
.addAll(calciteJdbcRules)
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
index fde3896..c49c629 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
@@ -31,17 +31,19 @@ import org.apache.calcite.plan.RelTraitSet;
import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexNode;
+import java.util.List;
+
/**
* Base class for logical and physical Limits implemented in Drill
*/
public abstract class DrillLimitRelBase extends SingleRel implements DrillRelNode {
protected RexNode offset;
protected RexNode fetch;
- private boolean pushDown; // whether limit has been pushed past its child.
- // Limit is special in that when it's pushed down, the original LIMIT still remains.
- // Once the limit is pushed down, this flag will be TRUE for the original LIMIT
- // and be FALSE for the pushed down LIMIT.
- // This flag will prevent optimization rules to fire in a loop.
+ private final boolean pushDown; // whether limit has been pushed past its child.
+ // Limit is special in that when it's pushed down, the original LIMIT still remains.
+ // Once the limit is pushed down, this flag will be TRUE for the original LIMIT
+ // and be FALSE for the pushed down LIMIT.
+ // This flag will prevent optimization rules to fire in a loop.
public DrillLimitRelBase(RelOptCluster cluster, RelTraitSet traitSet, RelNode child, RexNode offset, RexNode fetch) {
this(cluster, traitSet, child, offset, fetch, false);
@@ -54,6 +56,8 @@ public abstract class DrillLimitRelBase extends SingleRel implements DrillRelNod
this.pushDown = pushDown;
}
+ public abstract RelNode copy(RelTraitSet traitSet, List<RelNode> inputs, boolean pushDown);
+
public RexNode getOffset() {
return this.offset;
}
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillLimitRel.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillLimitRel.java
index 4d376a8..a370367 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillLimitRel.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillLimitRel.java
@@ -48,6 +48,11 @@ public class DrillLimitRel extends DrillLimitRelBase implements DrillRel {
}
@Override
+ public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs, boolean pushDown) {
+ return new DrillLimitRel(getCluster(), traitSet, sole(inputs), offset, fetch, pushDown);
+ }
+
+ @Override
public LogicalOperator implement(DrillImplementor implementor) {
LogicalOperator inputOp = implementor.visitChild(this, 0, getInput());
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitPrel.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitPrel.java
index ccbff17..e735346 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitPrel.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitPrel.java
@@ -57,6 +57,11 @@ public class LimitPrel extends DrillLimitRelBase implements Prel {
}
@Override
+ public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs, boolean pushDown) {
+ return new LimitPrel(getCluster(), traitSet, sole(inputs), offset, fetch, pushDown, isPartitioned);
+ }
+
+ @Override
public PhysicalOperator getPhysicalOperator(PhysicalPlanCreator creator) throws IOException {
Prel child = (Prel) this.getInput();
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/enumerable/plan/DrillJdbcRuleBase.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/enumerable/plan/DrillJdbcRuleBase.java
index 1a2722d..be258a2 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/enumerable/plan/DrillJdbcRuleBase.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/enumerable/plan/DrillJdbcRuleBase.java
@@ -146,6 +146,12 @@ public abstract class DrillJdbcRuleBase extends ConverterRule {
}
@Override
+ public boolean matches(RelOptRuleCall call) {
+ DrillLimitRelBase limit = call.rel(0);
+ return !limit.isPushDown() && super.matches(call);
+ }
+
+ @Override
public RelNode convert(RelNode rel) {
DrillLimitRelBase limit = (DrillLimitRelBase) rel;
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rel/PluginLimitRel.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rel/PluginLimitRel.java
index 6902ce2..b66c634 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rel/PluginLimitRel.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rel/PluginLimitRel.java
@@ -36,8 +36,8 @@ import java.util.List;
public class PluginLimitRel extends DrillLimitRelBase implements PluginRel {
public PluginLimitRel(RelOptCluster cluster, RelTraitSet traitSet,
- RelNode child, RexNode offset, RexNode fetch) {
- super(cluster, traitSet, child, offset, fetch);
+ RelNode child, RexNode offset, RexNode fetch, boolean pushDown) {
+ super(cluster, traitSet, child, offset, fetch, pushDown);
assert getConvention() == child.getConvention();
}
@@ -48,7 +48,12 @@ public class PluginLimitRel extends DrillLimitRelBase implements PluginRel {
@Override
public PluginLimitRel copy(RelTraitSet traitSet, List<RelNode> inputs) {
- return new PluginLimitRel(getCluster(), traitSet, inputs.get(0), offset, fetch);
+ return new PluginLimitRel(getCluster(), traitSet, inputs.get(0), offset, fetch, isPushDown());
+ }
+
+ @Override
+ public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs, boolean pushDown) {
+ return new PluginLimitRel(getCluster(), traitSet, inputs.get(0), offset, fetch, pushDown);
}
@Override
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rule/PluginLimitRule.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rule/PluginLimitRule.java
index 858f094..14910f8 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rule/PluginLimitRule.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/plan/rule/PluginLimitRule.java
@@ -18,6 +18,7 @@
package org.apache.drill.exec.store.plan.rule;
import org.apache.calcite.plan.Convention;
+import org.apache.calcite.plan.RelOptRuleCall;
import org.apache.calcite.plan.RelTrait;
import org.apache.calcite.rel.RelNode;
import org.apache.drill.exec.planner.common.DrillLimitRelBase;
@@ -33,6 +34,12 @@ public class PluginLimitRule extends PluginConverterRule {
}
@Override
+ public boolean matches(RelOptRuleCall call) {
+ DrillLimitRelBase limit = call.rel(0);
+ return !limit.isPushDown() && super.matches(call);
+ }
+
+ @Override
public RelNode convert(RelNode rel) {
DrillLimitRelBase limit = (DrillLimitRelBase) rel;
@@ -52,6 +59,7 @@ public class PluginLimitRule extends PluginConverterRule {
limit.getTraitSet().replace(getOutConvention()),
input,
limit.getOffset(),
- limit.getFetch());
+ limit.getFetch(),
+ true);
}
}