Add exception to recognize cyclic relational graph

sap-contributions · Dec 12, 2023 · e0c79e4 · e0c79e4
1 parent a6729b7
commit e0c79e4
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 24 deletions.
diff --git a/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java b/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
@@ -826,6 +826,27 @@ public JdbcSort(
           offset, fetch);
     }
 
+    @Override public void replaceInput(
+        int ordinalInParent,
+        RelNode rel) {
+      checkCycle(this,this);
+      super.replaceInput(ordinalInParent,rel);
+    }
+
+    private static void checkCycle(RelNode node, RelNode search) {
+      List<RelNode> inputs = node.getInputs();
+      if ( inputs.isEmpty()) return;
+      for ( RelNode input : inputs) {
+        if ( input instanceof RelSubset ) {
+          input = ((RelSubset)input).getBestOrOriginal();
+        }
+        if ( input == search) {
+          throw new RuntimeException("Detected cyclic dependency in JdbcSort " + search);
+        }
+        checkCycle(input,search);
+      }
+    }
+
     @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
         RelMetadataQuery mq) {
       RelOptCost cost = super.computeSelfCost(planner, mq);

diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java b/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java
@@ -25,11 +25,9 @@
 import org.apache.calcite.plan.RelTrait;
 import org.apache.calcite.plan.RelTraitSet;
 import org.apache.calcite.plan.hep.HepRelVertex;
-import org.apache.calcite.rel.AbstractRelNode;
-import org.apache.calcite.rel.PhysicalNode;
-import org.apache.calcite.rel.RelNode;
-import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.*;
 import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.TableScan;
 import org.apache.calcite.rel.externalize.RelWriterImpl;
 import org.apache.calcite.rel.metadata.RelMetadataQuery;
 import org.apache.calcite.rel.type.RelDataType;
@@ -48,15 +46,7 @@
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Comparator;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -388,7 +378,7 @@ void add(RelNode rel) {
    */
   RelNode buildCheapestPlan(VolcanoPlanner planner) {
     CheapestPlanReplacer replacer = new CheapestPlanReplacer(planner);
-    final RelNode cheapest = replacer.visit(this, -1, null);
+    final RelNode cheapest = replacer.visit(this, -1);
 
     if (planner.getListener() != null) {
       RelOptListener.RelChosenEvent event =
@@ -600,13 +590,22 @@ private boolean visitRel(RelNode p) {
     return "RelSubset#" + set.id + '.' + getTraitSet();
   }
 
+
+  static class VisitedNode {
+    final RelNode node;
+    final boolean processed;
+    VisitedNode(RelNode node, boolean processed) {
+      this.node = node;
+      this.processed = processed;
+    }
+  }
   /**
    * Visitor which walks over a tree of {@link RelSet}s, replacing each node
    * with the cheapest implementation of the expression.
    */
   static class CheapestPlanReplacer {
     VolcanoPlanner planner;
-    final Map<Integer, RelNode> visited = new HashMap<>();
+    final Map<Integer, VisitedNode> visited = new LinkedHashMap<>();
 
     CheapestPlanReplacer(VolcanoPlanner planner) {
       super();
@@ -621,17 +620,20 @@ private static String traitDiff(RelTraitSet original, RelTraitSet desired) {
           .collect(Collectors.joining(", ", "[", "]"));
     }
 
-    public RelNode visit(
-        RelNode p,
-        int ordinal,
-        @Nullable RelNode parent) {
+    private RelNode visit(RelNode p, int ordinal) {
       final int pId = p.getId();
-      RelNode prevVisit = visited.get(pId);
+      VisitedNode prevVisit = visited.get(pId);
       if (prevVisit != null) {
         // return memoized result of previous visit if available
-        return prevVisit;
+        if ( !prevVisit.processed) {
+          throw new RuntimeException("Cyclic relation graph detected at " + p
+              + ". Already processed nodes are: "
+              + visited.values().stream()
+              .map(it-> it.node.toString())
+              .collect(Collectors.joining(", ")));
+        }
+        return prevVisit.node;
       }
-
       if (p instanceof RelSubset) {
         RelSubset subset = (RelSubset) p;
         RelNode cheapest = subset.best;
@@ -722,6 +724,7 @@ public RelNode visit(
         }
         p = cheapest;
       }
+      visited.put(pId, new VisitedNode(p,false)); // memoize that we are processing this node
 
       if (ordinal != -1) {
         if (planner.getListener() != null) {
@@ -737,7 +740,7 @@ public RelNode visit(
       List<RelNode> inputs = new ArrayList<>();
       for (int i = 0; i < oldInputs.size(); i++) {
         RelNode oldInput = oldInputs.get(i);
-        RelNode input = visit(oldInput, i, p);
+        RelNode input = visit(oldInput, i);
         inputs.add(input);
       }
       if (!inputs.equals(oldInputs)) {
@@ -746,7 +749,7 @@ public RelNode visit(
         planner.provenanceMap.put(
             p, new VolcanoPlanner.DirectProvenance(pOld));
       }
-      visited.put(pId, p); // memoize result for pId
+      visited.put(pId, new VisitedNode(p,true)); // memoize result for pId
       return p;
     }
   }