[CALCITE-3916] Support top-down rule apply and upper bound space pruning #1991

FatLittle · 2020-05-29T03:46:02Z

related to #1950

Introduce Top-Down rule applying strategy in VolcanoPlanner
Implement tasks and state transformations according to the design of Columnbia
Introduce a LowerBoundCost metadata to compute lower bound and used for pruning
Using a flag to control whether to use new strategy or not

FatLittle · 2020-05-29T04:34:09Z

core/src/main/java/org/apache/calcite/tools/Programs.java

@@ -266,15 +266,14 @@ public static Program standard() {
  public static Program standard(RelMetadataProvider metadataProvider) {
    final Program program1 =
        (planner, rel, requiredOutputTraits, materializations, lattices) -> {
-          planner.setRoot(rel);


Setting root before addMaterialization will make MaterializedViewRule always fail to match

FatLittle · 2020-05-29T04:40:25Z

core/src/test/resources/sql/misc.iq

@@ -378,14 +378,14 @@ where exists (select 1 from "hr"."emps" where "empid" < 0);
 (0 rows)

 !ok
-EnumerableCalc(expr#0..1=[{inputs}], deptno=[$t0])
+EnumerableCalc(expr#0..1=[{inputs}], deptno=[$t1])
  EnumerableNestedLoopJoin(condition=[true], joinType=[inner])


Some join factors are swapped because when rule applying order changes, the id of join factors also changed. And EnumerableNestedLoopJoin has a special handling that it prefers left branch having a lower id than the right branch.

FatLittle · 2020-05-29T04:44:39Z

kafka/src/test/java/org/apache/calcite/adapter/kafka/KafkaAdapterTest.java

@@ -73,6 +74,7 @@

  @Test void testFilterWithProject() {
    assertModel(MODEL)
+        .with(CalciteConnectionProperty.TOP_DOWN_OPT.camelName(), false)


skip using top-down optimization here. This case will produce another plan with the same cost as the expected plan
(Two candidates are both in the memo but the new best plan is produced first). But it will hang when executing the new plan. Still don't know why it hangs.

@XuQianJin-Stars Do you have any insight?

core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java

core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java

rkondakov · 2020-05-31T10:43:02Z

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java

+          clearProcessed(planner.getSet(parentRel));
+        }
+        if (subset == planner.root) {
+          tasks.push(new OptimizeGroup(subset, planner.infCost));


If I understand right, when two sets merged together the optimization task will be restarted from the root, i.e. from the very beginning. Am I right? If so, shouldn't the tasks queue be cleaned in that case? Or have I missed something here?

Task queue cannot be cleared here, because RuleMatchs are already popped from rule queue during ApplyRules. So ApplyRule tasks in the queue cannot be reproduced after cleaning.

The newly added task may rescan the root RelSet and schedule tasks for new relnodes. Duplicate tasks can be avoided as input RelSubsets still hold the previous optimizing status.

The only problem is that it may affect the rule applying sequence.

hsyuan · 2020-06-06T00:56:23Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+
+  public MetadataDef<LowerBoundCost> getDef() {
+    return BuiltInMetadata.LowerBoundCost.DEF;
+  }


It doesn't looks like suitable for metadata. Why not just integrate it with planner, and let it overridable?

Will, it is somewhat like a metadata.

Different subclass of RelNodes may have different logic, like AC, RelNode, SpoolNode, RelSubset ...

It needs a cache and the cache should be invalidated when its input is changed.

Currently every time we add a new relnode through rule transformation, the whole metadata cache will be invalidated, which is not helpful for caching lower bound value.

After CALCITE-2018 is resolved, I think the invalidation of metadata could be removed.

hsyuan · 2020-06-06T04:31:47Z

core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java

@@ -179,6 +180,7 @@

  @Test void testPushDownSort() {
    CalciteAssert.model(JdbcTest.SCOTT_MODEL)
+        .with(CalciteConnectionProperty.TOPDOWN_OPT.camelName(), false)


why do we need this?

In this case, the new planner is producing a plan with exactly the same cost.
However, the new plan will affect later tests' assertion. I didn't want to modified so many of them. So I added this line instead of modifying the expects.

hsyuan · 2020-06-06T04:46:33Z

core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java

-    if (needsConverter && !planner.topDownOpt) {
-      addConverters(subset, required, true);
+    if (needsConverter) {
+      addConverters(subset, required, !planner.topDownOpt);


hmm, that means whenever a new traitset is added, it will always add the enforcer if needed.

Yes, it is. There are reasons for adding converters eagerly:

The converters act as bridges between the working subset and other subsets. Currently, the OptimizeGroup task will schedule tasks for all the logical Rels in the RelSet while it skips physical Rels from other subsets. That's because we are confident that if other subset can be reached by a converter, the converter is already in the current working subset.

In tasks other than OptimizeGroup, when new physical Rels are added to the RelSet, the insertion of the converter is a signal for RuleDriver to schedule the optimizing tasks for the previous working subset.

The computation of lower bound requires the memo to be connected.

Maybe there are some other ways to achieve all this, but I cannot find a more pretty way now. I was thinking of checking the upper bound before adding the enforcer. But I cannot decide which place is suitable for adding the enforcer back when the upper bound is changed. So I leave it for further optimizations.

hsyuan · 2020-06-06T04:48:08Z

core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java

+  /**
+   * RelNode ids that is invoked passThrough method before
+   */
+  Set<Integer> passThroughCache;


We can use RelNode here, instead of rel id, for debug-ability.

Good Point. Will resolve it.

hsyuan · 2020-06-10T20:33:46Z

core/src/main/java/org/apache/calcite/plan/volcano/CascadeRuleQueue.java

+/**
+ * A rule queue that manage rule matches for cascade planner
+ */
+public class CascadeRuleQueue implements RuleQueue {


Why do we need a rule queue for top-down? Can't it just execute after matching?

The origin cascades planner fire rules by required. But in this design, rules are fired during registering and executed by required.
We are trying to use on-demand rule firing. But it requires more controls to avoid miss-trigger or re-trigger rules. Related works are still under testings

I will create another JIRA to support on-demand rule matching.

hsyuan · 2020-06-10T21:19:57Z

core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java

-      matchList.clear();
-    }
-  }
+public interface RuleQueue {


nit: let's remove public.

hsyuan · 2020-06-10T21:23:23Z

core/src/main/java/org/apache/calcite/plan/volcano/MultiPhasedRuleDriver.java

+ * <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 {


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

hsyuan · 2020-06-10T21:35:59Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+
+  private boolean isLogical(RelNode relNode) {
+    return relNode.getTraitSet().getTrait(ConventionTraitDef.INSTANCE)
+        == Convention.NONE;


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.

Oops, it's my typo. I should use planner.isLogical instead of a new method here. planner.isLogical is supposed to be overwritten by users.

hsyuan · 2020-06-10T21:38:37Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+
+  public RelOptCost getLowerBoundCost(AbstractConverter ac,
+      RelMetadataQuery mq, RelOptPlanner planner) {
+    return mq.getLowerBoundCost(ac.getInput(), planner);


Still use abstract converter?

Currently it is not possible to have AC here. But as a metadata handler，I tried to make its logic as complete as possible.

Nah, once you add it, it will be hard to remove it and will always maintain it. Let's remove it.

zinking · 2020-07-01T13:07:08Z

core/src/main/java/org/apache/calcite/plan/volcano/IterativeRuleDriver.java

+        } catch (VolcanoTimeoutException e) {
+          planner.canonize();
+          ruleQueue.phaseCompleted(phase);
+          break PLANNING;


does call drive recursively here yield the same semantic ?

this code is completely copied from previous findBestExpr method

@zinking break PLANNING; here means step out of the for loop. It is not a recursive call.

zinking · 2020-07-01T13:11:32Z

plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java

+                + "                    EnumerableCalc(expr#0..27=[{inputs}], expr#28=['1998Q1'], expr#29=[=($t15, $t28)], expr#30=['1998Q2'], expr#31=[=($t15, $t30)], expr#32=['1998Q3'], expr#33=[=($t15, $t32)], expr#34=[OR($t29, $t31, $t33)], proj#0..27=[{exprs}], $condition=[$t34]): rowcount = 18262.25, cumulative cost = {91311.25 rows, 4748186.0 cpu, 0.0 io}\n"
+                + "                      EnumerableTableScan(table=[[TPCDS, DATE_DIM]]): rowcount = 73049.0, cumulative cost = {73049.0 rows, 73050.0 cpu, 0.0 io}\n"
+                + "                    EnumerableTableScan(table=[[TPCDS, STORE_RETURNS]]): rowcount = 287514.0, cumulative cost = {287514.0 rows, 287515.0 cpu, 0.0 io}\n"
+                + "                EnumerableSort(sort0=[$31], sort1=[$43], dir0=[ASC], dir1=[ASC]): rowcount = 3.94888649445E9, cumulative cost = {7.900926597146687E9 rows, 2.163983100662313E13 cpu, 0.0 io}\n"


I can't derive that these two are equivalent, how did you guys do that ?

The only changes is that some HashJoin is changed to MergeJoin. But as you can see from the cost, the new plan do have a lower cost.
(
old cost = 1.2435775409784036E28
VS
new cost = 1.2430341380834431E28
)

chunweilei · 2020-07-06T08:44:52Z

core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java

+  /**
+   * RelNode ids that is invoked passThrough method before
+   */
+  Set<RelNode> passThroughCache;


Correct the comment too~

Hmm. I think it is better to store the passThroughCache to RelSet, where the key is the traitset, the value are relnodes.

I see no differences. Currently, RelSet + Traits = RelSubset

chunweilei · 2020-07-06T08:46:16Z

cassandra/src/test/java/org/apache/calcite/test/CassandraAdapterTest.java

-                + "  CassandraLimit(fetch=[2])\n"
-                + "    CassandraProject(tweet_id=[$2])\n"
+                + "  CassandraProject(tweet_id=[$2])\n"
+                + "    CassandraLimit(fetch=[2])\n"
                + "      CassandraFilter(condition=[=($0, '!PUBLIC!')])\n");
  }


Does the same cost lead to the change?

yes, the have the same cost. However, I do think in most case the new plan is better.

chunweilei · 2020-07-06T08:53:14Z

core/src/test/resources/sql/agg.iq

-    EnumerableAggregate(group=[{2, 3, 7}], groups=[[{2, 3, 7}, {3}]], EXPR$2=[MIN($5)], EXPR$3=[MAX($5)], $g=[GROUPING($3, $7, $2)])
+EnumerableAggregate(group=[{1}], EXPR$1=[COUNT($2, $0) FILTER $5], EXPR$2=[MIN($3) FILTER $6], EXPR$3=[MIN($4) FILTER $6])
+  EnumerableCalc(expr#0..5=[{inputs}], expr#6=[0], expr#7=[=($t5, $t6)], expr#8=[5], expr#9=[=($t5, $t8)], proj#0..4=[{exprs}], $g_0=[$t7], $g_5=[$t9])
+    EnumerableAggregate(group=[{2, 3, 7}], groups=[[{2, 3, 7}, {3}]], EXPR$2=[MIN($5)], EXPR$3=[MAX($5)], $g=[GROUPING($2, $3, $7)])
      EnumerableTableScan(table=[[scott, EMP]])


Is the change expected?

the diff lays in grouping keys sequence ( [2,3,7] vs [3,7,2]). They should be equivalent.

junzheng-zheng · 2020-07-07T05:14:28Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+      return null;
+    }
+
+    RelOptCost lowerBound = null;


If current group is not fully optimized and has no winner, there may be problems to get the lower bound here. Since the current group's optimization may affact the input groups of the current group. E.g., the current group's optimization may push down some require traits and maybe somewhere of the inputs subtrees may enhance there's costs. So if we get the lower bound here, it may be larger than the actual lower bound which may miss optimal plan

You‘re right. If any logical node in the same RelSet is not built, looking for the lower bound of current RelSubset's member is not valid. Will remove this logic

hsyuan · 2020-07-08T19:49:33Z

core/src/main/java/org/apache/calcite/plan/volcano/CascadesRuleQueue.java

+
+  private final VolcanoPlanner planner;
+
+  private final Map<Integer, List<VolcanoRuleMatch>> matches = new HashMap<>();


Use IdentityHashMap, the key can be RelNode.

hsyuan · 2020-07-08T20:58:46Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+
+  public MetadataDef<LowerBoundCost> getDef() {
+    return BuiltInMetadata.LowerBoundCost.DEF;
+  }


Currently every time we add a new relnode through rule transformation, the whole metadata cache will be invalidated, which is not helpful for caching lower bound value.

hsyuan · 2020-07-08T21:03:58Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdLowerBoundCost.java

+
+  public RelOptCost getLowerBoundCost(AbstractConverter ac,
+      RelMetadataQuery mq, RelOptPlanner planner) {
+    return mq.getLowerBoundCost(ac.getInput(), planner);


Nah, once you add it, it will be hard to remove it and will always maintain it. Let's remove it.

hsyuan · 2020-07-08T21:10:23Z

core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java

+
+  /** Returns whether to skip a match. This happens if any of the
+   * {@link RelNode}s have importance zero. */
+  boolean skipMatch(VolcanoRuleMatch match) {


Why not let RuleQueue be an Abstract class, and put these methods there?

Currently, the planner is in charge of matching rules and pruning nodes (by importances or other mechanism). So I think it is more natural that planner judges whether a rule match is validate or not.

By moving to Planner, it could be easier to overwrite the logic by users, adding customized pruning logic

hsyuan · 2020-07-08T22:21:40Z

core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java

+  protected RelOptCost upperBoundForInputs(
+      RelNode mExpr, RelOptCost upperBound) {
+    if (!upperBound.isInfinite()) {
+      RelOptCost rootCost = mExpr.computeSelfCost(this,


computeSelfCost is deprecated...

hsyuan · 2020-07-08T22:50:29Z

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java

+    if (!rel.getTraitSet().satisfies(group.getTraitSet())) {
+      RelNode passThroughRel = convert(rel, group);
+      if (passThroughRel == null) {
+        if (LOGGER.isDebugEnabled()) {


The check if (LOGGER.isDebugEnabled()) { is redundant.

hsyuan · 2020-07-08T22:50:41Z

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java

+      RelOptCost upperBound = group.upperBound;
+      RelOptCost upperForInput = planner.upperBoundForInputs(mExpr, upperBound);
+      if (upperForInput.isLe(planner.zeroCost)) {
+        if (LOGGER.isDebugEnabled()) {


All the check if (LOGGER.isDebugEnabled()) { in this class are redundant.

hsyuan · 2020-07-14T02:59:51Z

core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java

-   * information regarding the contents of this Map and how it is initialized.
-   */
-  private final Map<VolcanoPlannerPhase, Set<String>> phaseRuleMapping;
+public abstract class RuleQueue {


remove public

hsyuan · 2020-07-14T03:07:10Z

Yes, doing the final round review

… pruning in calcite

pruning in calcite: resolve some comments

… pruning in calcite: 1. use RelNode as cache key 2. fix minor bugs found during tests

… pruning in calcite: 1. resolve some comments 2. fix ut

… pruning in calcite: resolve some comments

… pruning in calcite: some minor reconstructions

hsyuan

LGTM. Thanks for the excellent work, @FatLittle!

pruning in calcite: add lower bound check for derive traits

danny0405 · 2020-07-14T07:29:33Z

core/src/main/java/org/apache/calcite/plan/volcano/RuleDriver.java

+
+  /**
+   * gets the rule queue
+   */


FatLittle commented May 29, 2020

View reviewed changes

hsyuan changed the title ~~[CALCITE-3916] Implement top-down rule applying and upper bound space pruning in calcite~~ [CALCITE-3916] Support top-down rule apply and upper bound space pruning May 29, 2020

amaliujia reviewed May 30, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java Outdated Show resolved Hide resolved

amaliujia reviewed May 30, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java Outdated Show resolved Hide resolved

rkondakov reviewed May 31, 2020

View reviewed changes

FatLittle force-pushed the odps-cascade-one-planner branch 2 times, most recently from ef9dae4 to f21f99d Compare June 2, 2020 04:42

hsyuan reviewed Jun 6, 2020

View reviewed changes

hsyuan added the slow-tests-needed label Jun 6, 2020

hsyuan reviewed Jun 6, 2020

View reviewed changes

hsyuan reviewed Jun 10, 2020

View reviewed changes

FatLittle force-pushed the odps-cascade-one-planner branch 3 times, most recently from 4c2f0f3 to 62e2f79 Compare June 11, 2020 12:46

zinking reviewed Jul 1, 2020

View reviewed changes

FatLittle force-pushed the odps-cascade-one-planner branch from ee751f5 to 308f360 Compare July 2, 2020 12:16

chunweilei reviewed Jul 6, 2020

View reviewed changes

FatLittle mentioned this pull request Jul 7, 2020

[CALCITE-3916] Implement top-down rule applying and upper bound space pruning #1950

Closed

junzheng-zheng reviewed Jul 7, 2020

View reviewed changes

FatLittle force-pushed the odps-cascade-one-planner branch from 2d79067 to 9127f69 Compare July 8, 2020 13:57

hsyuan reviewed Jul 8, 2020

View reviewed changes

hsyuan reviewed Jul 14, 2020

View reviewed changes

jinpeng.wjp added 7 commits July 14, 2020 12:11

[CALCITE-3916] Implement top-down rule applying and upper bound space…

2bb452d

… pruning in calcite

[CALCITE-3916] Implement top-down rule applying and upper bound space

40158cf

pruning in calcite: resolve some comments

[CALCITE-3916] Implement top-down rule applying and upper bound space…

eca416e

… pruning in calcite: 1. use RelNode as cache key 2. fix minor bugs found during tests

[CALCITE-3916] Implement top-down rule applying and upper bound space…

b7c08db

… pruning in calcite: 1. resolve some comments 2. fix ut

[CALCITE-3916] Implement top-down rule applying and upper bound space…

0cb207e

… pruning in calcite: resolve some comments

[CALCITE-3916] Implement top-down rule applying and upper bound space…

0b2be82

… pruning in calcite: resolve some comments

[CALCITE-3916] Implement top-down rule applying and upper bound space…

9001db1

… pruning in calcite: some minor reconstructions

FatLittle force-pushed the odps-cascade-one-planner branch from bef8b7b to 9001db1 Compare July 14, 2020 05:31

hsyuan approved these changes Jul 14, 2020

View reviewed changes

hsyuan added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 14, 2020

[CALCITE-3916] Implement top-down rule applying and upper bound space

71f1443

pruning in calcite: add lower bound check for derive traits

danny0405 reviewed Jul 14, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/plan/volcano/RuleDriver.java

/**

* gets the rule queue

*/

Copy link

Contributor

danny0405 Jul 14, 2020


		private final VolcanoPlanner planner;

		private final Map<Integer, List<VolcanoRuleMatch>> matches = new HashMap<>();

[CALCITE-3916] Support top-down rule apply and upper bound space pruning #1991

[CALCITE-3916] Support top-down rule apply and upper bound space pruning #1991

Conversation

FatLittle commented May 29, 2020

Choose a reason for hiding this comment

FatLittle May 29, 2020 • edited Loading

Choose a reason for hiding this comment

FatLittle May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jun 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jun 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FatLittle Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsyuan Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsyuan commented Jul 14, 2020

hsyuan left a comment

Choose a reason for hiding this comment

FatLittle May 29, 2020 •

edited

Loading

FatLittle May 29, 2020 •

edited

Loading

FatLittle Jun 1, 2020 •

edited

Loading

FatLittle Jun 6, 2020 •

edited

Loading

FatLittle Jun 6, 2020 •

edited

Loading

FatLittle Jun 11, 2020 •

edited

Loading

FatLittle Jul 7, 2020 •

edited

Loading

FatLittle Jul 7, 2020 •

edited

Loading

FatLittle Jul 7, 2020 •

edited

Loading

hsyuan Jul 8, 2020 •

edited

Loading