You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/24 08:30:30 UTC

[GitHub] [calcite] chunweilei opened a new pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

chunweilei opened a new pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825
 
 
   JIRA: https://issues.apache.org/jira/browse/CALCITE-3817.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] chunweilei commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825#discussion_r383130171
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##########
 @@ -239,8 +237,8 @@
 
   /** Maps rule classes to their name, to ensure that the names are unique and
    * conform to rules. */
-  private final SetMultimap<String, Class> ruleNames =
-      LinkedHashMultimap.create();
+  private final Map<String, RelOptRule> ruleNames = new HashMap<>();
 
 Review comment:
   No need to use ```LinkedHashMultimap``` because the key should be unique.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] xndai commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825#discussion_r383445450
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##########
 @@ -451,12 +449,11 @@ public boolean addRule(RelOptRule rule) {
     assert added;
 
     final String ruleName = rule.toString();
-    if (ruleNames.put(ruleName, rule.getClass())) {
-      Set<Class> x = ruleNames.get(ruleName);
-      if (x.size() > 1) {
-        throw new RuntimeException("Rule description '" + ruleName
-            + "' is not unique; classes: " + x);
-      }
+    if (ruleNames.containsKey(ruleName)) {
 
 Review comment:
   You can simplify this as -
   
   if (ruleNames.put(ruleName, rule)) {
      ...
   }
   
   then you won't need the else block.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] chunweilei merged pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] chunweilei commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825#discussion_r383603505
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##########
 @@ -451,12 +449,11 @@ public boolean addRule(RelOptRule rule) {
     assert added;
 
     final String ruleName = rule.toString();
-    if (ruleNames.put(ruleName, rule.getClass())) {
-      Set<Class> x = ruleNames.get(ruleName);
-      if (x.size() > 1) {
-        throw new RuntimeException("Rule description '" + ruleName
-            + "' is not unique; classes: " + x);
-      }
+    if (ruleNames.containsKey(ruleName)) {
 
 Review comment:
   Good idea~

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] xndai commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825#discussion_r383446024
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##########
 @@ -493,6 +490,9 @@ public boolean removeRule(RelOptRule rule) {
       return false;
     }
 
+    // Remove rule name.
+    ruleNames.remove(rule.toString());
+
 
 Review comment:
   It's interesting that this is only called during planner clean up phase. But I think it's still good to fix this.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] chunweilei commented on issue #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule

Posted by GitBox <gi...@apache.org>.
chunweilei commented on issue #1825: [CALCITE-3817] VolcanoPlanner does not remove the entry in ruleNames when removing a rule
URL: https://github.com/apache/calcite/pull/1825#issuecomment-590865440
 
 
   Thank you for review, @xndai @hsyuan @rubenada ~

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services