This is an automated email from the ASF dual-hosted git repository.
hyuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new f95f74a [CALCITE-2166] Cumulative cost of RelSubset.best RelNode is
increased after calling RelSubset.propagateCostImprovements() for input
RelNodes (Xiening Dai)
f95f74a is described below
commit f95f74a13a20413bb0074f0a3c94901a7a88305c
Author: Xiening Dai <[email protected]>
AuthorDate: Wed Sep 4 10:44:27 2019 -0700
[CALCITE-2166] Cumulative cost of RelSubset.best RelNode is increased after
calling RelSubset.propagateCostImprovements() for input RelNodes (Xiening Dai)
It's possible that Subset's best cost increases when input subset's best is
changed. In those cases, although input subset's cost is reduced, the row
count
can increase which causes the increase of non-cumulative cost of parent
rel. As
a result, the cost of parent rel can increase. If the parent rel happens to
be
the best rel of a given subset, we currently do nothing. And this would
lead to
the inconsistency of the rel node cost.
Fixing this by updating the best rel node cost if it's increased. Although
this
approach won't garantee an optimal plan, at least it makes sure the memo is
consistent.
More details, please refer to JIRA -
https://issues.apache.org/jira/browse/CALCITE-2166
Close #1440
---
.../org/apache/calcite/plan/volcano/RelSubset.java | 26 +++++++++++++++++-----
.../calcite/plan/volcano/VolcanoPlanner.java | 19 ++++++++++++----
.../apache/calcite/test/ScannableTableTest.java | 4 ++--
3 files changed, 38 insertions(+), 11 deletions(-)
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
index 6accad3..7c78f71 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java
@@ -344,15 +344,31 @@ public class RelSubset extends AbstractRelNode {
return;
}
try {
- final RelOptCost cost = planner.getCost(rel, mq);
- if (cost.isLt(bestCost)) {
- LOGGER.trace("Subset cost improved: subset [{}] cost was {} now {}",
this, bestCost, cost);
+ RelOptCost cost = planner.getCost(rel, mq);
+ boolean updateBest = cost.isLt(bestCost);
+
+ // Best rel's cost is increased, we need to search for new best rel
+ if (rel == best && bestCost.isLt(cost)) {
+ updateBest = true;
+ for (RelNode node : getRels()) {
+ RelOptCost relCost = planner.getCost(node, mq);
+ if (relCost.isLt(cost)) {
+ cost = relCost;
+ rel = node;
+ }
+ }
+ }
+
+ // Update subset best cost when we find a cheaper rel or the current
+ // best's cost is changed
+ if (updateBest) {
+ LOGGER.trace("Subset cost changed: subset [{}] cost was {} now {}",
+ this, bestCost, cost);
bestCost = cost;
best = rel;
- // Lower cost means lower importance. Other nodes will change
- // too, but we'll get to them later.
+ // Recompute subset's importance and propagate cost change to parents
planner.ruleQueue.recompute(this);
for (RelNode parent : getParents()) {
final RelSubset parentSubset = planner.getSubset(parent);
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
index 43f79c4..36f6385 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
@@ -883,10 +883,21 @@ public class VolcanoPlanner extends AbstractRelOptPlanner
{
subset.getDescription(), set);
}
- // Make sure best RelNode is valid
- if (subset.best != null && !subset.set.rels.contains(subset.best)) {
- return litmus.fail("RelSubset [{}] does not contain its best RelNode
[{}]",
- subset.getDescription(), subset.best.getDescription());
+ if (subset.best != null) {
+
+ // Make sure best RelNode is valid
+ if (!subset.set.rels.contains(subset.best)) {
+ return litmus.fail("RelSubset [{}] does not contain its best
RelNode [{}]",
+ subset.getDescription(), subset.best.getDescription());
+ }
+
+ // Make sure bestCost is up-to-date
+ RelOptCost bestCost = getCost(subset.best,
subset.best.getCluster().getMetadataQuery());
+ if (!subset.bestCost.equals(bestCost)) {
+ return litmus.fail("RelSubset [" + subset.getDescription()
+ + "] has wrong best cost "
+ + subset.bestCost + ". Correct cost is " +
bestCost);
+ }
}
for (RelNode rel : subset.getRels()) {
diff --git a/core/src/test/java/org/apache/calcite/test/ScannableTableTest.java
b/core/src/test/java/org/apache/calcite/test/ScannableTableTest.java
index a6ea539..d627954 100644
--- a/core/src/test/java/org/apache/calcite/test/ScannableTableTest.java
+++ b/core/src/test/java/org/apache/calcite/test/ScannableTableTest.java
@@ -268,11 +268,11 @@ public class ScannableTableTest {
+ "group by \"k\"";
final Table table = new BeatlesProjectableFilterableTable(buf, false);
final String explain = "PLAN="
- + "EnumerableAggregate(group=[{0}], C=[COUNT()])\n"
+ + "EnumerableAggregate(group=[{1}], C=[COUNT()])\n"
+ " EnumerableAggregate(group=[{0, 1}])\n"
+ " EnumerableInterpreter\n"
+ " BindableTableScan(table=[[s, beatles]], "
- + "filters=[[=($2, 1940)]], projects=[[2, 0]])";
+ + "filters=[[=($2, 1940)]], projects=[[0, 2]])";
CalciteAssert.that()
.with(newSchema("s", "beatles", table))
.query(sql)
