From 57e60dfef8a75694dc1edb7598fe4b18c4466ea3 Mon Sep 17 00:00:00 2001 From: Hanumath Rao Maduri Date: Wed, 6 Jun 2018 20:48:12 -0700 Subject: [PATCH 1/2] DRILL-6476: Generate explain plan which shows relation between Lateral and the corresponding Unnest. --- .../planner/physical/PhysicalPlanCreator.java | 5 -- .../exec/planner/physical/UnnestPrel.java | 3 + .../physical/explain/NumberingRelWriter.java | 58 +++++++++++++++++-- .../impl/lateraljoin/TestLateralPlans.java | 21 +++++++ 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PhysicalPlanCreator.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PhysicalPlanCreator.java index 6a94662e78d..220add66fb5 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PhysicalPlanCreator.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PhysicalPlanCreator.java @@ -52,11 +52,6 @@ public QueryContext getContext() { return context; } -// public int getOperatorId(Prel prel){ -// OpId id = opIdMap.get(prel); -// return id.getAsSingleInt(); -// } - public PhysicalOperator addMetadata(Prel originalPrel, PhysicalOperator op){ op.setOperatorId(opIdMap.get(originalPrel).getAsSingleInt()); op.setCost(originalPrel.estimateRowCount(originalPrel.getCluster().getMetadataQuery())); diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/UnnestPrel.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/UnnestPrel.java index cd598eb9891..a22beea0e3e 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/UnnestPrel.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/UnnestPrel.java @@ -75,4 +75,7 @@ public boolean needsFinalColumnReordering() { return true; } + public Class getParentClass() { + return CorrelatePrel.class; + } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java index 045dba9d48e..7d1ac669d43 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java @@ -19,6 +19,7 @@ import java.io.PrintWriter; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -31,8 +32,10 @@ import org.apache.calcite.runtime.FlatLists; import org.apache.calcite.sql.SqlExplainLevel; import org.apache.calcite.util.Pair; +import org.apache.drill.exec.planner.physical.CorrelatePrel; import org.apache.drill.exec.planner.physical.HashJoinPrel; import org.apache.drill.exec.planner.physical.Prel; +import org.apache.drill.exec.planner.physical.UnnestPrel; import org.apache.drill.exec.planner.physical.explain.PrelSequencer.OpId; import com.google.common.collect.ImmutableList; @@ -47,6 +50,7 @@ class NumberingRelWriter implements RelWriter { private final SqlExplainLevel detailLevel; protected final Spacer spacer = new Spacer(); private final List> values = new ArrayList<>(); + private final Map registry; private final Map ids; //~ Constructors ----------------------------------------------------------- @@ -55,6 +59,7 @@ public NumberingRelWriter(Map ids, PrintWriter pw, SqlExplainLevel d this.pw = pw; this.ids = ids; this.detailLevel = detailLevel; + this.registry = new HashMap<>(); } //~ Methods ---------------------------------------------------------------- @@ -71,7 +76,7 @@ protected void explain_( RelMetadataQuery mq = RelMetadataQuery.instance(); if (!mq.isVisibleInExplain(rel, detailLevel)) { // render children in place of this, at same level - explainInputs(inputs); + explainInputs(rel); return; } @@ -95,6 +100,7 @@ protected void explain_( s.append(rel.getRelTypeName().replace("Prel", "")); if (detailLevel != SqlExplainLevel.NO_ATTRIBUTES) { int j = 0; + s.append(getDependentSrcOp(rel)); for (Pair value : values) { if (value.right instanceof RelNode) { continue; @@ -125,14 +131,56 @@ protected void explain_( } pw.println(s); spacer.add(2); - explainInputs(inputs); + explainInputs(rel); spacer.subtract(2); } - private void explainInputs(List inputs) { - for (RelNode input : inputs) { - input.explain(this); + private String getDependentSrcOp(RelNode rel) { + if (rel instanceof UnnestPrel) { + return this.getDependentSrcOp((UnnestPrel) rel); } + return ""; + } + + private String getDependentSrcOp(UnnestPrel unnest) { + Prel parent = this.getRegisteredPrel(unnest.getParentClass()); + if (parent != null && parent instanceof CorrelatePrel) { + OpId id = ids.get(parent); + return String.format(" [SrcOp: (%02d-%02d)] ", id.fragmentId, id.opId); + } + return ""; + } + + public void register(Prel toRegister) { + this.registry.put(toRegister.getClass().getSimpleName(), toRegister); + } + + public Prel getRegisteredPrel(Class classname) { + return this.registry.get(classname.getSimpleName()); + } + + public void unRegister(Prel unregister) { + this.registry.remove(unregister.getClass().getSimpleName()); + } + + + private void explainInputs(RelNode rel) { + if (rel instanceof CorrelatePrel) { + this.explainInputs((CorrelatePrel) rel); + } else { + for (RelNode input : rel.getInputs()) { + input.explain(this); + } + } + } + + //Correlate is handled differently because explain plan + //needs to show relation between Lateral and Unnest operators. + private void explainInputs(CorrelatePrel correlate) { + correlate.getInput(0).explain(this); + this.register(correlate); + correlate.getInput(1).explain(this); + this.unRegister(correlate); } public final void explain(RelNode rel, List> valueList) { diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java index 00ab971e150..d9ba29a84c8 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java @@ -435,4 +435,25 @@ private String getRightChildOfLateral(String explain) throws Exception { String CorrelateUnnest = matcher.group(0); return CorrelateUnnest.substring(CorrelateUnnest.lastIndexOf("Scan")); } + + + //The following test is for testing the explain plan contains relation between lateral and corresponding unnest. + @Test + public void testLateralAndUnnestExplainPlan() throws Exception { + String Sql = "select c.* from cp.`lateraljoin/nested-customer.json` c, unnest(c.orders) Orders(ord)"; + ClusterFixtureBuilder builder = ClusterFixture.builder(dirTestWatcher) + .setOptionDefault(ExecConstants.ENABLE_UNNEST_LATERAL_KEY, true) + .setOptionDefault(ExecConstants.SLICE_TARGET, 1); + + try (ClusterFixture cluster = builder.build(); + ClientFixture client = cluster.clientFixture()) { + String explain = client.queryBuilder().sql(Sql).explainText(); + String srcOp = explain.substring(explain.indexOf("SrcOp")); + assertTrue(srcOp != null && srcOp.length() > 0); + String correlateFragmentPattern = srcOp.substring(srcOp.indexOf("(")+1, srcOp.indexOf(")")); + assertTrue(correlateFragmentPattern != null && correlateFragmentPattern.length() > 0); + Matcher matcher = Pattern.compile(correlateFragmentPattern + ".*Correlate", Pattern.MULTILINE | Pattern.DOTALL).matcher(explain); + assertTrue(matcher.find()); + } + } } From 408cb38368872205d558cbb06a89500fe2943d49 Mon Sep 17 00:00:00 2001 From: Hanumath Rao Maduri Date: Mon, 11 Jun 2018 10:21:18 -0700 Subject: [PATCH 2/2] Addressing review comments. --- .../physical/explain/NumberingRelWriter.java | 25 +++++++++---------- .../impl/lateraljoin/TestLateralPlans.java | 4 +-- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java index 7d1ac669d43..38b97b67d6e 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/explain/NumberingRelWriter.java @@ -50,7 +50,7 @@ class NumberingRelWriter implements RelWriter { private final SqlExplainLevel detailLevel; protected final Spacer spacer = new Spacer(); private final List> values = new ArrayList<>(); - private final Map registry; + private final Map sourceOperatorRegistry; private final Map ids; //~ Constructors ----------------------------------------------------------- @@ -59,7 +59,7 @@ public NumberingRelWriter(Map ids, PrintWriter pw, SqlExplainLevel d this.pw = pw; this.ids = ids; this.detailLevel = detailLevel; - this.registry = new HashMap<>(); + this.sourceOperatorRegistry = new HashMap<>(); } //~ Methods ---------------------------------------------------------------- @@ -67,12 +67,6 @@ public NumberingRelWriter(Map ids, PrintWriter pw, SqlExplainLevel d protected void explain_( RelNode rel, List> values) { - List inputs = rel.getInputs(); - if (rel instanceof HashJoinPrel && ((HashJoinPrel) rel).isSwapped()) { - HashJoinPrel joinPrel = (HashJoinPrel) rel; - inputs = FlatLists.of(joinPrel.getRight(), joinPrel.getLeft()); - } - RelMetadataQuery mq = RelMetadataQuery.instance(); if (!mq.isVisibleInExplain(rel, detailLevel)) { // render children in place of this, at same level @@ -146,21 +140,21 @@ private String getDependentSrcOp(UnnestPrel unnest) { Prel parent = this.getRegisteredPrel(unnest.getParentClass()); if (parent != null && parent instanceof CorrelatePrel) { OpId id = ids.get(parent); - return String.format(" [SrcOp: (%02d-%02d)] ", id.fragmentId, id.opId); + return String.format(" [srcOp=%02d-%02d] ", id.fragmentId, id.opId); } return ""; } public void register(Prel toRegister) { - this.registry.put(toRegister.getClass().getSimpleName(), toRegister); + this.sourceOperatorRegistry.put(toRegister.getClass().getSimpleName(), toRegister); } public Prel getRegisteredPrel(Class classname) { - return this.registry.get(classname.getSimpleName()); + return this.sourceOperatorRegistry.get(classname.getSimpleName()); } public void unRegister(Prel unregister) { - this.registry.remove(unregister.getClass().getSimpleName()); + this.sourceOperatorRegistry.remove(unregister.getClass().getSimpleName()); } @@ -168,7 +162,12 @@ private void explainInputs(RelNode rel) { if (rel instanceof CorrelatePrel) { this.explainInputs((CorrelatePrel) rel); } else { - for (RelNode input : rel.getInputs()) { + List inputs = rel.getInputs(); + if (rel instanceof HashJoinPrel && ((HashJoinPrel) rel).isSwapped()) { + HashJoinPrel joinPrel = (HashJoinPrel) rel; + inputs = FlatLists.of(joinPrel.getRight(), joinPrel.getLeft()); + } + for (RelNode input : inputs) { input.explain(this); } } diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java index d9ba29a84c8..d027e77afdb 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/lateraljoin/TestLateralPlans.java @@ -448,9 +448,9 @@ public void testLateralAndUnnestExplainPlan() throws Exception { try (ClusterFixture cluster = builder.build(); ClientFixture client = cluster.clientFixture()) { String explain = client.queryBuilder().sql(Sql).explainText(); - String srcOp = explain.substring(explain.indexOf("SrcOp")); + String srcOp = explain.substring(explain.indexOf("srcOp")); assertTrue(srcOp != null && srcOp.length() > 0); - String correlateFragmentPattern = srcOp.substring(srcOp.indexOf("(")+1, srcOp.indexOf(")")); + String correlateFragmentPattern = srcOp.substring(srcOp.indexOf("=")+1, srcOp.indexOf("]")); assertTrue(correlateFragmentPattern != null && correlateFragmentPattern.length() > 0); Matcher matcher = Pattern.compile(correlateFragmentPattern + ".*Correlate", Pattern.MULTILINE | Pattern.DOTALL).matcher(explain); assertTrue(matcher.find());