[CALCITE-2018] Queries failed with AssertionError: rel has lower cost…

… than best cost of subset
apache · Apr 16, 2019 · dfa8317 · dfa8317
1 parent 9538374
commit dfa8317
Show file tree

Hide file tree

Showing 16 changed files with 87 additions and 43 deletions.
diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCalc.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCalc.java
@@ -143,7 +143,7 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
             inputJavaType);
 
     final RexBuilder rexBuilder = getCluster().getRexBuilder();
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = getCluster().getMetadataQuery();
     final RelOptPredicateList predicates = mq.getPulledUpPredicates(child);
     final RexSimplify simplify =
         new RexSimplify(rexBuilder, predicates, RexUtil.EXECUTOR);

diff --git a/core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java b/core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
@@ -248,7 +248,7 @@ public RelOptCost getCost(RelNode rel, RelMetadataQuery mq) {
 
   @SuppressWarnings("deprecation")
   public RelOptCost getCost(RelNode rel) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
     return getCost(rel, mq);
   }
 

diff --git a/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java b/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java
@@ -35,6 +35,7 @@
 import org.apache.calcite.rel.convert.ConverterRule;
 import org.apache.calcite.rel.convert.TraitMatchingRule;
 import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.rel.metadata.RelMdUtil;
 import org.apache.calcite.rel.metadata.RelMetadataProvider;
 import org.apache.calcite.rel.metadata.RelMetadataQuery;
 import org.apache.calcite.util.Pair;
@@ -866,6 +867,7 @@ private void contractVertices(
         }
         parentRel.replaceInput(i, preservedVertex);
       }
+      RelMdUtil.clearCache(parentRel);
       graph.removeEdge(parent, discardedVertex);
       graph.addEdge(parent, preservedVertex);
       updateVertex(parent, parentRel);
@@ -927,8 +929,9 @@ private RelNode buildFinalPlan(HepRelVertex vertex) {
       }
       child = buildFinalPlan((HepRelVertex) child);
       rel.replaceInput(i, child);
-      rel.recomputeDigest();
     }
+    RelMdUtil.clearCache(rel);
+    rel.recomputeDigest();
 
     return rel;
   }

diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
@@ -33,7 +33,9 @@
 
 import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -311,21 +313,24 @@ void mergeWith(
     assert otherSet.equivalentSet == null;
     LOGGER.trace("Merge set#{} into set#{}", otherSet.id, id);
     otherSet.equivalentSet = this;
+    RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
 
     // remove from table
     boolean existed = planner.allSets.remove(otherSet);
     assert existed : "merging with a dead otherSet";
 
+    Map<RelSubset, RelNode> changedSubsets = new IdentityHashMap<>();
+
     // merge subsets
     for (RelSubset otherSubset : otherSet.subsets) {
       planner.ruleQueue.subsetImportances.remove(otherSubset);
       RelSubset subset =
           getOrCreateSubset(
               otherSubset.getCluster(),
               otherSubset.getTraitSet());
+      // collect RelSubset instances, whose best should be changed
       if (otherSubset.bestCost.isLt(subset.bestCost)) {
-        subset.bestCost = otherSubset.bestCost;
-        subset.best = otherSubset.best;
+        changedSubsets.put(subset, otherSubset.best);
       }
       for (RelNode otherRel : otherSubset.getRels()) {
         planner.reregister(this, otherRel);
@@ -335,6 +340,18 @@ void mergeWith(
     // Has another set merged with this?
     assert equivalentSet == null;
 
+    // calls propagateCostImprovements() for RelSubset instances,
+    // whose best should be changed to check whether that
+    // subset's parents get cheaper.
+    Set<RelSubset> activeSet = new HashSet<>();
+    for (Map.Entry<RelSubset, RelNode> subsetBestPair : changedSubsets.entrySet()) {
+      RelSubset relSubset = subsetBestPair.getKey();
+      relSubset.propagateCostImprovements(
+          planner, mq, subsetBestPair.getValue(),
+          activeSet);
+    }
+    assert activeSet.isEmpty();
+
     // Update all rels which have a child in the other set, to reflect the
     // fact that the child has been renamed.
     //
@@ -353,8 +370,6 @@ void mergeWith(
     }
 
     // Make sure the cost changes as a result of merging are propagated.
-    final Set<RelSubset> activeSet = new HashSet<>();
-    final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
     for (RelNode parentRel : getParentRels()) {
       final RelSubset parentSubset = planner.getSubset(parentRel);
       parentSubset.propagateCostImprovements(

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
@@ -208,7 +208,8 @@ Set<RelNode> getParents() {
     final Set<RelNode> list = new LinkedHashSet<>();
     for (RelNode parent : set.getParentRels()) {
       for (RelSubset rel : inputSubsets(parent)) {
-        if (rel.set == set && traitSet.satisfies(rel.getTraitSet())) {
+        // see usage of this method in propagateCostImprovements0()
+        if (rel == this) {
           list.add(parent);
         }
       }
@@ -350,12 +351,17 @@ void propagateCostImprovements0(VolcanoPlanner planner, RelMetadataQuery mq,
 
         bestCost = cost;
         best = rel;
+        // since best was changed, cached metadata for this subset should be removed
+        mq.clearCache(this);
 
         // Lower cost means lower importance. Other nodes will change
         // too, but we'll get to them later.
         planner.ruleQueue.recompute(this);
         for (RelNode parent : getParents()) {
+          // removes parent cached metadata since its input was changed
+          mq.clearCache(parent);
           final RelSubset parentSubset = planner.getSubset(parent);
+          // parent subset will clear its cache in propagateCostImprovements0 method itself
           parentSubset.propagateCostImprovements(planner, mq, parent,
               activeSet);
         }

diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
@@ -46,6 +46,7 @@
 import org.apache.calcite.rel.convert.ConverterRule;
 import org.apache.calcite.rel.externalize.RelWriterImpl;
 import org.apache.calcite.rel.metadata.JaninoRelMetadataProvider;
+import org.apache.calcite.rel.metadata.RelMdUtil;
 import org.apache.calcite.rel.metadata.RelMetadataProvider;
 import org.apache.calcite.rel.metadata.RelMetadataQuery;
 import org.apache.calcite.rel.rules.AggregateJoinTransposeRule;
@@ -1570,6 +1571,7 @@ private boolean fixUpInputs(RelNode rel) {
         }
       }
     }
+    RelMdUtil.clearCache(rel);
     return changeCount > 0;
   }
 

diff --git a/core/src/main/java/org/apache/calcite/prepare/CalciteMaterializer.java b/core/src/main/java/org/apache/calcite/prepare/CalciteMaterializer.java
@@ -21,8 +21,8 @@
 import org.apache.calcite.interpreter.BindableConvention;
 import org.apache.calcite.jdbc.CalcitePrepare;
 import org.apache.calcite.jdbc.CalciteSchema;
+import org.apache.calcite.plan.RelOptCluster;
 import org.apache.calcite.plan.RelOptMaterialization;
-import org.apache.calcite.plan.RelOptPlanner;
 import org.apache.calcite.plan.RelOptTable;
 import org.apache.calcite.plan.RelOptUtil;
 import org.apache.calcite.rel.RelNode;
@@ -62,10 +62,10 @@ class CalciteMaterializer extends CalcitePrepareImpl.CalcitePreparingStmt {
   CalciteMaterializer(CalcitePrepareImpl prepare,
       CalcitePrepare.Context context,
       CatalogReader catalogReader, CalciteSchema schema,
-      RelOptPlanner planner, SqlRexConvertletTable convertletTable) {
+      SqlRexConvertletTable convertletTable, RelOptCluster cluster) {
     super(prepare, context, catalogReader, catalogReader.getTypeFactory(),
-        schema, EnumerableRel.Prefer.ANY, planner, BindableConvention.INSTANCE,
-        convertletTable);
+        schema, EnumerableRel.Prefer.ANY, BindableConvention.INSTANCE,
+        convertletTable, cluster);
   }
 
   /** Populates a materialization record, converting a table path

diff --git a/core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java b/core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
@@ -313,8 +313,8 @@ private ParseResult convert_(Context context, String sql, boolean analyze,
 
     final CalcitePreparingStmt preparingStmt =
         new CalcitePreparingStmt(this, context, catalogReader, typeFactory,
-            context.getRootSchema(), null, planner, resultConvention,
-            createConvertletTable());
+            context.getRootSchema(), null, resultConvention, createConvertletTable(),
+            createCluster(planner, new RexBuilder(typeFactory)));
     final SqlToRelConverter converter =
         preparingStmt.getSqlToRelConverter(validator, catalogReader,
             configBuilder.build());
@@ -721,8 +721,8 @@ <T> CalciteSignature<T> prepare2_(
             : EnumerableConvention.INSTANCE;
     final CalcitePreparingStmt preparingStmt =
         new CalcitePreparingStmt(this, context, catalogReader, typeFactory,
-            context.getRootSchema(), prefer, planner, resultConvention,
-            createConvertletTable());
+            context.getRootSchema(), prefer, resultConvention, createConvertletTable(),
+            createCluster(planner, new RexBuilder(typeFactory)));
 
     final RelDataType x;
     final Prepare.PreparedResult preparedResult;
@@ -988,7 +988,7 @@ private static String getTypeName(RelDataType type) {
   }
 
   protected void populateMaterializations(Context context,
-      RelOptPlanner planner, Prepare.Materialization materialization) {
+      Prepare.Materialization materialization, RelOptCluster cluster) {
     // REVIEW: initialize queryRel and tableRel inside MaterializationService,
     // not here?
     try {
@@ -1000,8 +1000,8 @@ protected void populateMaterializations(Context context,
               context.getTypeFactory(),
               context.config());
       final CalciteMaterializer materializer =
-          new CalciteMaterializer(this, context, catalogReader, schema, planner,
-              createConvertletTable());
+          new CalciteMaterializer(this, context, catalogReader, schema,
+              createConvertletTable(), cluster);
       materializer.populate(materialization);
     } catch (Exception e) {
       throw new RuntimeException("While populating materialization "
@@ -1053,6 +1053,7 @@ static class CalcitePreparingStmt extends Prepare
     protected final RelDataTypeFactory typeFactory;
     protected final SqlRexConvertletTable convertletTable;
     private final EnumerableRel.Prefer prefer;
+    private final RelOptCluster cluster;
     private final Map<String, Object> internalParameters =
         new LinkedHashMap<>();
     private int expansionDepth;
@@ -1064,17 +1065,18 @@ static class CalcitePreparingStmt extends Prepare
         RelDataTypeFactory typeFactory,
         CalciteSchema schema,
         EnumerableRel.Prefer prefer,
-        RelOptPlanner planner,
         Convention resultConvention,
-        SqlRexConvertletTable convertletTable) {
+        SqlRexConvertletTable convertletTable,
+        RelOptCluster cluster) {
       super(context, catalogReader, resultConvention);
       this.prepare = prepare;
       this.schema = schema;
       this.prefer = prefer;
-      this.planner = planner;
+      this.cluster = cluster;
+      this.planner = cluster.getPlanner();
+      this.rexBuilder = cluster.getRexBuilder();
       this.typeFactory = typeFactory;
       this.convertletTable = convertletTable;
-      this.rexBuilder = new RexBuilder(typeFactory);
     }
 
     @Override protected void init(Class runtimeContextClass) {
@@ -1143,7 +1145,6 @@ private PreparedResult prepare_(Supplier<RelNode> fn,
         SqlValidator validator,
         CatalogReader catalogReader,
         SqlToRelConverter.Config config) {
-      final RelOptCluster cluster = prepare.createCluster(planner, rexBuilder);
       return new SqlToRelConverter(this, validator, catalogReader, cluster,
           convertletTable, config);
     }
@@ -1279,7 +1280,7 @@ public Type getElementType() {
               ? MaterializationService.instance().query(schema)
               : ImmutableList.of();
       for (Prepare.Materialization materialization : materializations) {
-        prepare.populateMaterializations(context, planner, materialization);
+        prepare.populateMaterializations(context, materialization, cluster);
       }
       return materializations;
     }

diff --git a/core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java b/core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
@@ -158,13 +158,13 @@ public String getCorrelVariable() {
 
   @SuppressWarnings("deprecation")
   public boolean isDistinct() {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = cluster.getMetadataQuery();
     return Boolean.TRUE.equals(mq.areRowsUnique(this));
   }
 
   @SuppressWarnings("deprecation")
   public boolean isKey(ImmutableBitSet columns) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = cluster.getMetadataQuery();
     return Boolean.TRUE.equals(mq.areColumnsUnique(this, columns));
   }
 
@@ -236,7 +236,7 @@ public List<RelNode> getInputs() {
 
   @SuppressWarnings("deprecation")
   public final double getRows() {
-    return estimateRowCount(RelMetadataQuery.instance());
+    return estimateRowCount(cluster.getMetadataQuery());
   }
 
   public double estimateRowCount(RelMetadataQuery mq) {
@@ -278,7 +278,7 @@ public RelNode accept(RexShuttle shuttle) {
 
   @SuppressWarnings("deprecation")
   public final RelOptCost computeSelfCost(RelOptPlanner planner) {
-    return computeSelfCost(planner, RelMetadataQuery.instance());
+    return computeSelfCost(planner, cluster.getMetadataQuery());
   }
 
   public RelOptCost computeSelfCost(RelOptPlanner planner,

diff --git a/core/src/main/java/org/apache/calcite/rel/core/Filter.java b/core/src/main/java/org/apache/calcite/rel/core/Filter.java
@@ -138,13 +138,13 @@ public RexNode getCondition() {
 
   @Deprecated // to be removed before 2.0
   public static double estimateFilteredRows(RelNode child, RexProgram program) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = child.getCluster().getMetadataQuery();
     return RelMdUtil.estimateFilteredRows(child, program, mq);
   }
 
   @Deprecated // to be removed before 2.0
   public static double estimateFilteredRows(RelNode child, RexNode condition) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = child.getCluster().getMetadataQuery();
     return RelMdUtil.estimateFilteredRows(child, condition, mq);
   }
 

diff --git a/core/src/main/java/org/apache/calcite/rel/core/Join.java b/core/src/main/java/org/apache/calcite/rel/core/Join.java
@@ -189,7 +189,7 @@ public JoinRelType getJoinType() {
   public static double estimateJoinedRows(
       Join joinRel,
       RexNode condition) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = joinRel.getCluster().getMetadataQuery();
     return Util.first(RelMdUtil.getJoinRowCount(mq, joinRel, condition), 1D);
   }
 

diff --git a/core/src/main/java/org/apache/calcite/rel/core/Union.java b/core/src/main/java/org/apache/calcite/rel/core/Union.java
@@ -62,7 +62,7 @@ protected Union(RelInput input) {
 
   @Deprecated // to be removed before 2.0
   public static double estimateRowCount(RelNode rel) {
-    final RelMetadataQuery mq = RelMetadataQuery.instance();
+    final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
     return RelMdUtil.getUnionAllRowCount(mq, (Union) rel);
   }
 }

diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/JaninoRelMetadataProvider.java b/core/src/main/java/org/apache/calcite/rel/metadata/JaninoRelMetadataProvider.java
@@ -265,10 +265,9 @@ private static <M extends Metadata> MetadataHandler<M> load3(
             .append(method.i)
             .append(")");
       }
-      buff.append(", r");
       safeArgList(buff, method.e)
           .append(");\n")
-          .append("    final Object v = mq.map.get(key);\n")
+          .append("    final Object v = mq.map.get(r, key);\n")
           .append("    if (v != null) {\n")
           .append("      if (v == ")
           .append(NullSentinel.class.getName())
@@ -286,7 +285,7 @@ private static <M extends Metadata> MetadataHandler<M> load3(
           .append(method.e.getReturnType().getName())
           .append(") v;\n")
           .append("    }\n")
-          .append("    mq.map.put(key,")
+          .append("    mq.map.put(r, key,")
           .append(NullSentinel.class.getName())
           .append(".ACTIVE);\n")
           .append("    try {\n")
@@ -297,14 +296,14 @@ private static <M extends Metadata> MetadataHandler<M> load3(
           .append("_(r, mq");
       argList(buff, method.e)
           .append(");\n")
-          .append("      mq.map.put(key, ")
+          .append("      mq.map.put(r, key, ")
           .append(NullSentinel.class.getName())
           .append(".mask(x));\n")
           .append("      return x;\n")
           .append("    } catch (")
           .append(Exception.class.getName())
           .append(" e) {\n")
-          .append("      mq.map.remove(key);\n")
+          .append("      mq.map.row(r).clear();\n")
           .append("      throw e;\n")
           .append("    }\n")
           .append("  }\n")

diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java b/core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
@@ -178,7 +178,7 @@ private static RelMetadataProvider reflectiveSource(
                   }
                   key1 = FlatLists.copyOf(args2);
                 }
-                if (mq.map.put(key1, NullSentinel.INSTANCE) != null) {
+                if (mq.map.put(rel, key1, NullSentinel.INSTANCE) != null) {
                   throw CyclicMetadataException.INSTANCE;
                 }
                 try {
@@ -188,7 +188,7 @@ private static RelMetadataProvider reflectiveSource(
                   Util.throwIfUnchecked(e.getCause());
                   throw new RuntimeException(e.getCause());
                 } finally {
-                  mq.map.remove(key1);
+                  mq.map.remove(rel, key1);
                 }
               });
       methodsMap.put(key, function);