[GitHub] [calcite] liyafan82 commented on a change in pull request #2501: [CALCITE-4747] clean all outdated edges from graph.edges

[GitBox <gi...@apache.org>]

liyafan82 commented on a change in pull request #2501:
URL: https://github.com/apache/calcite/pull/2501#discussion_r703348416



##########
File path: core/src/test/java/org/apache/calcite/test/HepPlannerTest.java
##########
@@ -267,6 +271,37 @@ private void assertIncludesExactlyOnce(String message, String digest, String sub
     assertThat(listener.getApplyTimes() == 1, is(true));
   }
 
+  @Test void testCleanEdges() {
+    // this case is designed to test
+    // CALCITE-4747:https://issues.apache.org/jira/browse/CALCITE-4747
+    // before the improvement, the following optimization will have 7 edges in the graph,
+    // and now there's only 4 edges.
+    HepProgramBuilder programBuilder = HepProgram.builder();
+    programBuilder.addRuleInstance(CoreRules.FILTER_INTO_JOIN);
+
+    final HepTestListener listener = new HepTestListener(0);
+    HepPlanner planner = new HepPlanner(programBuilder.build());
+    planner.addListener(listener);
+
+    final String sql = "select * from emp e " +
+        "join dept d on e.deptno=d.deptno " +
+        "where e.empno>100";
+
+    try {
+      planner.setRoot(tester.convertSqlToRel(sql).rel);
+      planner.findBestExp();
+
+      // the field graph is private, so we use reflect to get it.
+      Field graphField = planner.getClass().getDeclaredField("graph");
+      graphField.setAccessible(true);
+      DirectedGraph<HepRelVertex, DefaultEdge> graph =
+          (DirectedGraph<HepRelVertex, DefaultEdge>) graphField.get(planner);
+      assertEquals(4, graph.edgeSet().size());
+    } catch (Exception e) {
+      throw new RuntimeException("there's unknown error happened in the case testCleanEdges:" + e.getMessage());
+    }
+  }
+

Review comment:
       Thanks for the update. 
   IMO, we need a more direct test case. For example, we can:
   1) construct a graph, 
   2) verify the number of edges in the graph, 
   3) call `removeAllVertices` on the graph
   4) verify the number of edges again (it should be reduced)
   
   Maybe you can find an example in `org.apache.calcite.util.graph.DirectedGraphTest`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org