hsyuan commented on a change in pull request #1991:
URL: https://github.com/apache/calcite/pull/1991#discussion_r438414610



##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -16,349 +16,18 @@
  */
 package org.apache.calcite.plan.volcano;
 
-import org.apache.calcite.plan.RelOptRuleOperand;
-import org.apache.calcite.rel.RelNode;
-import org.apache.calcite.rel.rules.SubstitutionRule;
-import org.apache.calcite.util.Util;
-import org.apache.calcite.util.trace.CalciteTrace;
-
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimap;
-
-import org.slf4j.Logger;
-
-import java.io.PrintWriter;
-import java.io.StringWriter;
-import java.util.ArrayDeque;
-import java.util.Deque;
-import java.util.EnumMap;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.Map;
-import java.util.Queue;
-import java.util.Set;
-
-/**
- * Priority queue of relexps whose rules have not been called, and rule-matches
- * which have not yet been acted upon.
- */
-class RuleQueue {
-  //~ Static fields/initializers ---------------------------------------------
-
-  private static final Logger LOGGER = CalciteTrace.getPlannerTracer();
-
-  private static final Set<String> ALL_RULES = ImmutableSet.of("<ALL RULES>");
-
-  //~ Instance fields --------------------------------------------------------
-
-  /**
-   * Map of {@link VolcanoPlannerPhase} to a list of rule-matches. Initially,
-   * there is an empty {@link PhaseMatchList} for each planner phase. As the
-   * planner invokes {@link #addMatch(VolcanoRuleMatch)} the rule-match is
-   * added to the appropriate PhaseMatchList(s). As the planner completes
-   * phases, the matching entry is removed from this list to avoid unused
-   * work.
-   */
-  final Map<VolcanoPlannerPhase, PhaseMatchList> matchListMap =
-      new EnumMap<>(VolcanoPlannerPhase.class);
-
-  private final VolcanoPlanner planner;
-
-  /**
-   * Maps a {@link VolcanoPlannerPhase} to a set of rule descriptions. Named 
rules
-   * may be invoked in their corresponding phase.
-   *
-   * <p>See {@link VolcanoPlannerPhaseRuleMappingInitializer} for more
-   * information regarding the contents of this Map and how it is initialized.
-   */
-  private final Map<VolcanoPlannerPhase, Set<String>> phaseRuleMapping;
-
-  //~ Constructors -----------------------------------------------------------
-
-  RuleQueue(VolcanoPlanner planner) {
-    this.planner = planner;
-
-    phaseRuleMapping = new EnumMap<>(VolcanoPlannerPhase.class);
-
-    // init empty sets for all phases
-    for (VolcanoPlannerPhase phase : VolcanoPlannerPhase.values()) {
-      phaseRuleMapping.put(phase, new HashSet<>());
-    }
-
-    // configure phases
-    planner.getPhaseRuleMappingInitializer().initialize(phaseRuleMapping);
-
-    for (VolcanoPlannerPhase phase : VolcanoPlannerPhase.values()) {
-      // empty phases get converted to "all rules"
-      if (phaseRuleMapping.get(phase).isEmpty()) {
-        phaseRuleMapping.put(phase, ALL_RULES);
-      }
-
-      // create a match list data structure for each phase
-      PhaseMatchList matchList = new PhaseMatchList(phase);
-
-      matchListMap.put(phase, matchList);
-    }
-  }
-
-  //~ Methods ----------------------------------------------------------------
-  /**
-   * Clear internal data structure for this rule queue.
-   */
-  public void clear() {
-    for (PhaseMatchList matchList : matchListMap.values()) {
-      matchList.clear();
-    }
-  }
+public interface RuleQueue {

Review comment:
       nit: let's remove public.

##########
File path: 
core/src/main/java/org/apache/calcite/plan/volcano/MultiPhasedRuleDriver.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.calcite.plan.volcano;
+
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.trace.CalciteTrace;
+
+import org.slf4j.Logger;
+
+/***
+ * <p>The algorithm executes repeatedly in a series of phases. In each phase
+ * the exact rules that may be fired varies. The mapping of phases to rule
+ * sets is maintained in the {@link #ruleQueue}.
+ *
+ * <p>In each phase, the planner then iterates over the rule matches presented
+ * by the rule queue until the rule queue becomes empty.
+ */
+public class MultiPhasedRuleDriver implements RuleDriver {

Review comment:
       MultiPhasedRuleDriver -> IterativeRuleDriver
   The multiphase doesn't work well at all. No body use it, we should consider 
removing multi phases in volcano planner.
   and remove public

##########
File path: 
core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.calcite.rel.metadata;
+
+import org.apache.calcite.plan.Convention;
+import org.apache.calcite.plan.ConventionTraitDef;
+import org.apache.calcite.plan.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.volcano.AbstractConverter;
+import org.apache.calcite.plan.volcano.RelSubset;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.metadata.BuiltInMetadata.LowerBoundCost;
+import org.apache.calcite.util.BuiltInMethod;
+
+/**
+ * Default implementations of the
+ * {@link BuiltInMetadata.LowerBoundCost}
+ * metadata provider for the standard algebra.
+ */
+public class RelMdLowerBoundCost implements MetadataHandler<LowerBoundCost> {
+
+  public static final RelMetadataProvider SOURCE =
+      ReflectiveRelMetadataProvider.reflectiveSource(
+          new RelMdLowerBoundCost(), BuiltInMethod.LOWER_BOUND_COST.method);
+
+  //~ Constructors -----------------------------------------------------------
+
+  protected RelMdLowerBoundCost() {}
+
+  //~ Methods ----------------------------------------------------------------
+
+  public MetadataDef<LowerBoundCost> getDef() {
+    return BuiltInMetadata.LowerBoundCost.DEF;
+  }
+
+  private boolean isLogical(RelNode relNode) {
+    return relNode.getTraitSet().getTrait(ConventionTraitDef.INSTANCE)
+        == Convention.NONE;

Review comment:
       There may be other usages in down stream project that defines their own 
logical conventions, while still keep NONE. That means they are using 2 logical 
conventions......
   But let's keep it simple for NONE for now.

##########
File path: 
core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.calcite.rel.metadata;
+
+import org.apache.calcite.plan.Convention;
+import org.apache.calcite.plan.ConventionTraitDef;
+import org.apache.calcite.plan.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.volcano.AbstractConverter;
+import org.apache.calcite.plan.volcano.RelSubset;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.metadata.BuiltInMetadata.LowerBoundCost;
+import org.apache.calcite.util.BuiltInMethod;
+
+/**
+ * Default implementations of the
+ * {@link BuiltInMetadata.LowerBoundCost}
+ * metadata provider for the standard algebra.
+ */
+public class RelMdLowerBoundCost implements MetadataHandler<LowerBoundCost> {
+
+  public static final RelMetadataProvider SOURCE =
+      ReflectiveRelMetadataProvider.reflectiveSource(
+          new RelMdLowerBoundCost(), BuiltInMethod.LOWER_BOUND_COST.method);
+
+  //~ Constructors -----------------------------------------------------------
+
+  protected RelMdLowerBoundCost() {}
+
+  //~ Methods ----------------------------------------------------------------
+
+  public MetadataDef<LowerBoundCost> getDef() {
+    return BuiltInMetadata.LowerBoundCost.DEF;
+  }
+
+  private boolean isLogical(RelNode relNode) {
+    return relNode.getTraitSet().getTrait(ConventionTraitDef.INSTANCE)
+        == Convention.NONE;
+  }
+
+  public RelOptCost getLowerBoundCost(RelSubset subset,
+      RelMetadataQuery mq, RelOptPlanner planner) {
+
+    if (isLogical(subset)) {
+      // currently only support physical, will improve in the future
+      return null;
+    }
+
+    RelOptCost winner = subset.getWinnerCost();
+    if (winner != null) {
+      // when this subset is fully optimized, just return the winner
+      return winner;
+    }
+
+    // if group is not fully explored. Its properties like cardinality
+    // would get changed after exploration. So it cannot return a valid LB
+    if (!subset.isExplored()) {
+      return null;
+    }
+
+    RelOptCost lowerBound = null;
+    for (RelNode relNode : subset.getRels()) {
+      try {
+        RelOptCost lb = mq.getLowerBoundCost(relNode, planner);
+        if (lb == null) {
+          return null;
+        }
+        if (lowerBound == null || lb.isLt(lowerBound)) {
+          lowerBound = lb;
+        }
+      } catch (CyclicMetadataException e) {
+        if (lowerBound == null) {
+          // a cyclic metadata query means this node has an INF LB
+          lowerBound = planner.getCostFactory().makeInfiniteCost();
+        }
+      }
+    }
+    return lowerBound;
+  }
+
+  public RelOptCost getLowerBoundCost(RelNode node,
+      RelMetadataQuery mq, RelOptPlanner planner) {
+    if (isLogical(node)) {
+      // currently only support physical, will improve in the future
+      return null;
+    }
+
+    RelOptCost selfCost = node.computeSelfCost(planner, mq);
+    if (selfCost.isInfinite()) {
+      selfCost = null;
+    }
+    for (RelNode input : node.getInputs()) {
+      RelOptCost lb = mq.getLowerBoundCost(input, planner);
+      if (lb != null) {
+        selfCost = selfCost == null ? lb : selfCost.plus(lb);
+      }
+    }
+    return selfCost;
+  }
+
+  public RelOptCost getLowerBoundCost(AbstractConverter ac,
+      RelMetadataQuery mq, RelOptPlanner planner) {
+    return mq.getLowerBoundCost(ac.getInput(), planner);

Review comment:
       Still use abstract converter?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to