[CALCITE-3455] Redundant rule firing for both logical and physical nodes #1543

xndai · 2019-10-28T22:12:17Z

For ProjectMergeRule, EnumerableLimitRule and SortRemoveConstantKeysRule, once they are done with the transformation on logical nodes, there's no need to fire these rules on physical nodes again. So limit these rule matching to be with logical nodes only. This can noticeably reduce optimization latency.

danny0405 · 2019-10-29T02:25:18Z

core/src/main/java/org/apache/calcite/rel/rules/ProjectMergeRule.java

-        operand(Project.class,
-            operand(Project.class, any())),
+        operand(LogicalProject.class,
+            operand(LogicalProject.class, any())),


The changes sounds reasonable to me, but i'm a little worried about the compatibility. It would give us more confidence if some engine like Druid can give feedback that this change does not break something

Do you know who is the right contact for Druid?

I think @zabetak can make some help ~

danny0405 · 2019-10-30T01:55:31Z

Quotes Julian's comment in the mailing list:

Is there a change we could make to the rule API that would make it easier to customize rules? I have been thinking for a while about adding an “operand builder” to replace the static methods (e.g. RelOptRule.operandJ) that we use to build the operands inside rules. Maybe as part of this change, we could allow rules to clone themselves with slightly different parameters.

I’m interested in hearing about a “big change” that would save us from a myriad of small changes in the coming years.

I'm also expecting if we can make more effort to do a bigger refactoring to make the RelOptRule customization more easier.

jinxing64 · 2019-10-30T02:49:38Z

core/src/main/java/org/apache/calcite/rel/rules/SortRemoveConstantKeysRule.java

@@ -44,7 +45,7 @@

  private SortRemoveConstantKeysRule() {


Thanks a lot ~
Besides all rules in existing change (FilterProjectTransposeRule, ProjectMergeRule, SortRemoveConstantKeyRule), shall we include more rules, which only need to be applied for logical nodes ?

I am confident that the rules I update here will not cause the change of search space, aka the transformation on physical nodes would result in an equivalent, so it's safe. But we need to be careful when updating other rules. And it'd better be done by the rule author. This change would solve most, if not all, the problems, and we can always come back to update other rules when feel necessary.

Thanks for explain ~

xndai · 2019-10-30T05:30:40Z

Quotes Julian's comment in the mailing list:

Is there a change we could make to the rule API that would make it easier to customize rules? I have been thinking for a while about adding an “operand builder” to replace the static methods (e.g. RelOptRule.operandJ) that we use to build the operands inside rules. Maybe as part of this change, we could allow rules to clone themselves with slightly different parameters.

I’m interested in hearing about a “big change” that would save us from a myriad of small changes in the coming years.

I'm also expecting if we can make more effort to do a bigger refactoring to make the RelOptRule customization more easier.

If I understand correctly, what Julian's proposing is to refactor the RelOptRule interface so it's easier to specify which class of RelNode to match. In general I agree with this idea, but it's orthogonal to what I am fixing here. If we had such interface in place, we would still need to pass in logical class rather than the base class to avoid duplicating rule firing, which is what I am doing here.

rubenada · 2019-10-30T09:03:11Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableLimitRule.java

@@ -30,7 +31,7 @@
 class EnumerableLimitRule extends RelOptRule {
  EnumerableLimitRule() {
    super(
-        operand(Sort.class, any()),
+        operand(LogicalSort.class, any()),


Please update the javadoc accordingly (org.apache.calcite.rel.logical.LogicalSort instead of org.apache.calcite.rel.core.Sort). The same for the rest of the modified rules.

Good catch. Fixed. Thx.

xndai · 2019-11-12T17:13:00Z

@danny0405 had a good feedback - since ProjectMergeRule can also be used by HepPlanner, and in some cases user would use HepPlanner to merge physical project nodes at the end of optimization phase, so I update this change to only disable physical project merge under volcano planer.

danny0405 · 2019-11-13T01:43:29Z

core/src/test/java/org/apache/calcite/test/enumerable/EnumerableCorrelateTest.java

            + "    EnumerableAggregate(group=[{0}])\n"
            + "      EnumerableTableScan(table=[[s, depts]])\n"
-            + "    EnumerableCalc(expr#0..4=[{inputs}], expr#5=[100], expr#6=[>($t0, $t5)], expr#7=[$cor5], expr#8=[$t7.deptno], expr#9=[=($t1, $t8)], expr#10=[AND($t6, $t9)], proj#0..2=[{exprs}], $condition=[$t10])\n"
+            + "    EnumerableCalc(expr#0..4=[{inputs}], expr#5=[$cor4], expr#6=[$t5.deptno], expr#7=[=($t1, $t6)], expr#8=[100], expr#9=[>($t0, $t8)], expr#10=[AND($t7, $t9)], proj#0..2=[{exprs}], $condition=[$t10])\n"


Thanks @xndai , can you explain why this plan was changed ?

The new plan is equivalent. The rule firing sequence is changed now, so we get an alternative plan.

zabetak · 2019-11-13T08:09:13Z

core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java

@@ -60,7 +62,7 @@
   * {@link org.apache.calcite.rel.core.Correlate} from being de-correlated.
   */
  public static final FilterProjectTransposeRule INSTANCE =
-      new FilterProjectTransposeRule(Filter.class, Project.class, true, true,
+      new FilterProjectTransposeRule(LogicalFilter.class, LogicalProject.class, true, true,


This is a backwards incompatible change. Please add an entry in the history.md (see breaking changes).

Would it make sense (to keep backwards compatibility) to leave the rule's "default" INSTANCE as it is (matching Filter+Project), and create a separate e.g. LOGICAL_INSTANCE matching LogicalFilter+LogicalProject?

This is a good idea. I will try that. Thanks @rubenada

zabetak · 2019-11-13T08:12:58Z

core/src/main/java/org/apache/calcite/rel/rules/ProjectMergeRule.java

+    // We only merge logical project nodes with VolcanoPlanner. Since LogicalProject
+    // would be converted into physical nodes later. There's no need to fire this
+    // rule on physical project nodes again.
+    if (call.getPlanner() instanceof VolcanoPlanner


I'm not in favor of having planner specific behavior inside the rules. I think creating two instances of the rule and passing them to different planners/rulesets is a cleaner solution.

Yes, I agree with @zabetak.

zabetak · 2019-11-13T08:18:03Z

core/src/main/java/org/apache/calcite/rel/rules/SortRemoveConstantKeysRule.java

@@ -44,7 +45,7 @@

  private SortRemoveConstantKeysRule() {
    super(
-        operand(Sort.class, any()),
+        operand(LogicalSort.class, any()),


Again this is a backwards incompatible change. Other than that I am more in favor of creating different instances of the rule rather than limiting the rule to match only LogicalSort. We have seen projects were people create their own set of logical operators. Changing the rule like this makes it impossible for such projects to use the rule.

Exactly. The rule did work for the projects that inherit from Sort, however, it would stop working.

Thanks for pointing this out. I am working on a revision.

vlsi · 2019-11-13T15:57:12Z

Have you seen https://issues.apache.org/jira/browse/CALCITE-2223 ?
I think 3455 duplicates 2223, and, unfortunately, there's no trivial solution for it.

xndai · 2019-11-14T21:52:07Z

Have you seen https://issues.apache.org/jira/browse/CALCITE-2223 ?
I think 3455 duplicates 2223, and, unfortunately, there's no trivial solution for it.

I don't think this is a duplication. This patch tries to improve the optimization performance by avoid firing redundant rules. It's not particularly related to cycles between RelSubset.

For EnumerableLimitRule and SortRemoveConstantKeysRule, once they are done with the transformation on logical nodes, there's no need to fire these rules on physical nodes again. And for ProjectMergeRule, ProjectFilterTransposeRule and FilterProjectTransposeRule, we add logical instances which only match logical nodes. We keep this existing instances unchanged for backward compatability purpose.

…LimitRule

…ing to logical nodes only

xndai · 2019-12-03T21:54:36Z

@vlsi @zabetak @rubenada @hsyuan I've updated this patch based on your feedback. Now I create separate LOGICAL_INSTANCE for rules that potentially would only match for logical nodes. Now Calcite users can customize the rule set and use the LOGICAL_INSTANCE if they want. Also updated some unit tests to use the LOGICAL_INSTANCE instead. Please take a look.

jinxing64 · 2019-12-04T02:38:02Z

I have a question related to CALCITE-3455 but not necessarily related to this PR:
How about providing an approach for RelOptRule and allow to specify that rels matched should have the same convention (without specifying what that convention should be) ? IMHO we can also save a lot of redundant fires by this way and I think lots of existing rules can work well with this fashion.

xndai · 2019-12-04T05:28:50Z

I have a question related to CALCITE-3455 but not necessarily related to this PR:
How about providing an approach for RelOptRule and allow to specify that rels matched should have the same convention (without specifying what that convention should be) ? IMHO we can also save a lot of redundant fires by this way and I think lots of existing rules can work well with this fashion.

I agree in general this is the right direction. Not only the rule should allow specific convention for matching, but also able to create new rels with given conventions. For example, today when we fire ProjectMerge, it will generate another LogicalProject even if two EmmuerableProjects are merged. This doesn't make sense. But in order to do that we should provide a way to register RelFactory into the framework for custom convention. This mechanism is also needed by CALCITE-2970.

Having said that, for short term / mid term I feel we can alleviate the problem by restricting certain rules to only match logic nodes, which is what this PR intends to do.

zabetak · 2019-12-04T07:33:11Z

There was a discussion in the dev list about rules matching different conventions but we didn't reach consensus at that time.

xndai · 2019-12-04T23:56:03Z

Thanks @zabetak for the pointer. After reviewing the whole thread, I now understand why @vlsi believes this is dup of 2223.

I think "single convention" and "convention preserving" rules are the concepts that capture exactly what we need.

For "single convention", although user can specify operand class, some rules today don't have extension points, e.g. ProjectMergeRule. Anyone who would want to update those rule to only match a given convention would have to copy and rewrite the whole rule. Or at least sub-class the existing rules.

And then there's no way to support "convention preserving" as the actual convention needs to be defined when defining the rule.

Personally I agree with @vlsi that this is one outstanding missing piece of Calcite which affects planning performance quite a bit. Most of the Calcite users would just use the default rule set, and expect them to work out of the box. It would be too much to ask them to rewrite or even sub-class the rules.

My patch here provides a temporary solution to address the most pressing need - those rules I updated do get fired a lot in practice. But if we can reach a conclusion on 2223 (looks like we are very close), this patch won't be needed.

danny0405 reviewed Oct 29, 2019

View reviewed changes

hsyuan changed the title ~~[Calcite-3455] Redundant rule firing for both logical and physical nodes~~ [CALCITE-3455] Redundant rule firing for both logical and physical nodes Oct 29, 2019

jinxing64 reviewed Oct 30, 2019

View reviewed changes

rubenada reviewed Oct 30, 2019

View reviewed changes

danny0405 added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Oct 30, 2019

danny0405 reviewed Nov 13, 2019

View reviewed changes

zabetak reviewed Nov 13, 2019

View reviewed changes

xndai force-pushed the calcite-3455 branch from eb10d34 to 9b64e01 Compare November 25, 2019 23:51

danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52

xndai added 4 commits December 2, 2019 16:23

Create LOGCIAL_INSTANCE for SortRemoveConstantKeysRule and Enumerable…

1e10492

…LimitRule

Only use the logical instances of the updated rules in unit test

4c5a39c

Revert changes on ProjectFilterTransposeRule since it's already apply…

6c8acf4

…ing to logical nodes only

xndai force-pushed the calcite-3455 branch from fb02da7 to 6c8acf4 Compare December 3, 2019 21:12

vlsi force-pushed the master branch 2 times, most recently from 49cb002 to 8768a23 Compare December 29, 2019 12:07

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:52

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:19

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-3455] Redundant rule firing for both logical and physical nodes #1543

[CALCITE-3455] Redundant rule firing for both logical and physical nodes #1543

xndai commented Oct 28, 2019 •

edited

Loading

danny0405 Oct 29, 2019

xndai Oct 29, 2019

danny0405 Oct 30, 2019

danny0405 commented Oct 30, 2019

jinxing64 Oct 30, 2019

xndai Oct 30, 2019 •

edited

Loading

jinxing64 Oct 30, 2019

xndai commented Oct 30, 2019

rubenada Oct 30, 2019

xndai Oct 30, 2019

xndai commented Nov 12, 2019

danny0405 Nov 13, 2019

xndai Nov 13, 2019

zabetak Nov 13, 2019

rubenada Nov 13, 2019

xndai Nov 14, 2019

zabetak Nov 13, 2019

hsyuan Nov 13, 2019

zabetak Nov 13, 2019

vlsi Nov 26, 2019

xndai Nov 26, 2019

vlsi commented Nov 13, 2019

xndai commented Nov 14, 2019

xndai commented Dec 3, 2019

jinxing64 commented Dec 4, 2019 •

edited

Loading

xndai commented Dec 4, 2019

zabetak commented Dec 4, 2019

xndai commented Dec 4, 2019

[CALCITE-3455] Redundant rule firing for both logical and physical nodes #1543

Are you sure you want to change the base?

[CALCITE-3455] Redundant rule firing for both logical and physical nodes #1543

Conversation

xndai commented Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Oct 30, 2019

Choose a reason for hiding this comment

xndai Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xndai commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xndai commented Nov 12, 2019

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

vlsi commented Nov 13, 2019

xndai commented Nov 14, 2019

xndai commented Dec 3, 2019

jinxing64 commented Dec 4, 2019 • edited Loading

xndai commented Dec 4, 2019

zabetak commented Dec 4, 2019

xndai commented Dec 4, 2019

xndai commented Oct 28, 2019 •

edited

Loading

xndai Oct 30, 2019 •

edited

Loading

jinxing64 commented Dec 4, 2019 •

edited

Loading