[CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner by hsyuan · Pull Request #1869 · apache/calcite

hsyuan · 2020-03-23T23:30:25Z

JIRA: https://issues.apache.org/jira/browse/CALCITE-3868

chunweilei

Nice catch~

danny0405 · 2020-03-24T07:21:42Z

core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java

-        : "Rule's description should not contain '$': "
-        + description;
-    assert !INTEGER_PATTERN.matcher(description).matches()
-        : "Rule's description should not be an integer: "


How should we validate the rule description pattern now ?

When creating the rule

zabetak

Thanks for the PR @hsyuan ! I left some small comments. Apart from that, I think the history.md should be updated with a small note about the change in these public APIs.

zabetak · 2020-03-24T08:51:10Z

core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java

   * are unique.
   */
-  private final Map<String, RelOptRule> mapDescToRule = new HashMap<>();
+  protected final Map<String, RelOptRule> mapDescToRule = new HashMap<>();


With this change the map becomes the main data structure holding the rules so I think the Javadoc should be updated to reflect this. Increasing the visibility of the field makes it public API; it might be better to keep this private and rely on public/protected methods to recover the necessary info.

I think we may need to change it to LinkedHashMap to be consistent with the requirement of HepPlanner.

It turns out we still need it to be protected to avoid copying.

I think that in most cases the number of rules is not very big so I was thinking that copying vs. mapDescToRule.values() is not going to have significant performance overhead in the planning phase, thus, I tend to prefer better encapsulation. Having that said, I do not have any concrete measures to support my claims (just instinct that could be wrong) :)

I agree the rules is not very big. But in onNewClass, every time we add a new logical/physical operator we have to copy it. It will copy N times depends on how many operators you have. Though not a significant overhead, but I think we still need to avoid multiple times copy.

I'm fine with any option so I am leaving the final decision up to you :)

zabetak · 2020-03-24T08:54:00Z

core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java

-    // Check that there isn't a rule with the same description,
-    // also validating description string.
-
+  protected boolean mapRuleDescription(RelOptRule rule) {


The method now basically registers the rule to the planner so it might be better to rename this entirely to addRule.

zabetak · 2020-03-24T08:54:46Z

core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java

+   * @return the rule that is removed, or null if no rule is removed
   */
-  protected void unmapRuleDescription(RelOptRule rule) {
+  protected RelOptRule unmapRuleDescription(RelOptRule rule) {


Rename this to removeRule and update the Javadoc?

The parameter of this method is weird, should be string description of the rule, like removeRule(String desc) ?

zabetak · 2020-03-24T08:59:39Z

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

    this.provenanceMap.clear();
  }

  public List<RelOptRule> getRules() {


Would it be a good idea to move the method to AbstractRelOptPlanner? This would allow us to keep mapDescToRule private and replace calls to mapDescToRule.values() with calls to this method.

zabetak · 2020-03-24T09:01:36Z

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

+    return ImmutableList.copyOf(mapDescToRule.values());
  }

  public boolean addRule(RelOptRule rule) {


If we choose to add this method to the superclass (instead of having mapRuleDescription) then here we can ovverride.

@zabetak I have addressed your comments. Thanks.

neoremind · 2020-03-26T07:06:49Z

core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java

    if (instruction.ruleSet == null) {
      instruction.ruleSet = new LinkedHashSet<>();
-      for (RelOptRule rule : allRules) {
+      for (RelOptRule rule : mapDescToRule.values()) {


If we already know the size of mapDescToRule, we can create a set with exact size, which will eliminate capacity expansion overhead and space waste when creating. Even though this is trivial update, I think it is always a good manner to create collection in such way if possible.

instruction.ruleSet = Sets.newHashSetWithExpectedSize(mapDescToRule.size());

Yes, will do.

It turns out the size instruction.ruleSet is undetermined, we don't know the exact size. So I will leave as it is.

I agree, there is "if" check to determine what to be added. I didn't find that out clearly, sorry about the false alert.

hsyuan added the slow-tests-needed label Mar 23, 2020

chunweilei reviewed Mar 24, 2020

View reviewed changes

danny0405 reviewed Mar 24, 2020

View reviewed changes

zabetak reviewed Mar 24, 2020

View reviewed changes

neoremind reviewed Mar 26, 2020

View reviewed changes

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

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

[CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

010645a

hsyuan merged commit 90f3e98 into apache:master Apr 21, 2020

hsyuan deleted the remove_ruleset branch April 21, 2020 09:54

Conversation

hsyuan commented Mar 23, 2020

Uh oh!

chunweilei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zabetak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsyuan Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hsyuan Mar 25, 2020 •

edited

Loading