[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-28 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r399717476
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
 ##
 @@ -44,18 +44,13 @@
  * Abstract base for implementations of the {@link RelOptPlanner} interface.
  */
 public abstract class AbstractRelOptPlanner implements RelOptPlanner {
-  //~ Static fields/initializers -
-
-  /** Regular expression for integer. */
-  private static final Pattern INTEGER_PATTERN = Pattern.compile("[0-9]+");
-
   //~ Instance fields 
 
   /**
* Maps rule description to rule, just to ensure that rules' descriptions
* are unique.
*/
-  private final Map mapDescToRule = new HashMap<>();
+  protected final Map mapDescToRule = new HashMap<>();
 
 Review comment:
   I'm fine with any option so I am leaving the final decision up to you :)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-26 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r398410186
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
 ##
 @@ -44,18 +44,13 @@
  * Abstract base for implementations of the {@link RelOptPlanner} interface.
  */
 public abstract class AbstractRelOptPlanner implements RelOptPlanner {
-  //~ Static fields/initializers -
-
-  /** Regular expression for integer. */
-  private static final Pattern INTEGER_PATTERN = Pattern.compile("[0-9]+");
-
   //~ Instance fields 
 
   /**
* Maps rule description to rule, just to ensure that rules' descriptions
* are unique.
*/
-  private final Map mapDescToRule = new HashMap<>();
+  protected final Map mapDescToRule = new HashMap<>();
 
 Review comment:
   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) :)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-24 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r396992730
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##
 @@ -392,35 +382,24 @@ public RelSet getSet(RelNode rel) {
 this.mapRel2Subset.clear();
 this.relImportances.clear();
 this.ruleQueue.clear();
-this.ruleNames.clear();
 this.materializations.clear();
 this.latticeByName.clear();
 this.provenanceMap.clear();
   }
 
   public List getRules() {
 
 Review comment:
   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.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-24 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r396989637
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
 ##
 @@ -156,16 +142,18 @@ protected void mapRuleDescription(RelOptRule rule) {
 + "existing rule=" + existingRule + "; new rule=" + rule);
   }
 }
+return true;
   }
 
   /**
* Removes the mapping between a rule and its description.
*
* @param rule Rule
+   * @return the rule that is removed, or null if no rule is removed
*/
-  protected void unmapRuleDescription(RelOptRule rule) {
+  protected RelOptRule unmapRuleDescription(RelOptRule rule) {
 
 Review comment:
   Rename this to `removeRule` and update the Javadoc?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-24 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r396989167
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
 ##
 @@ -130,24 +125,15 @@ public void checkCancel() {
*
* @param rule Rule
*/
-  protected void mapRuleDescription(RelOptRule rule) {
-// Check that there isn't a rule with the same description,
-// also validating description string.
-
+  protected boolean mapRuleDescription(RelOptRule rule) {
 
 Review comment:
   The method now basically registers the rule to the planner so it might be 
better to rename this entirely to `addRule`. 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-24 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r396987477
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
 ##
 @@ -44,18 +44,13 @@
  * Abstract base for implementations of the {@link RelOptPlanner} interface.
  */
 public abstract class AbstractRelOptPlanner implements RelOptPlanner {
-  //~ Static fields/initializers -
-
-  /** Regular expression for integer. */
-  private static final Pattern INTEGER_PATTERN = Pattern.compile("[0-9]+");
-
   //~ Instance fields 
 
   /**
* Maps rule description to rule, just to ensure that rules' descriptions
* are unique.
*/
-  private final Map mapDescToRule = new HashMap<>();
+  protected final Map mapDescToRule = new HashMap<>();
 
 Review comment:
   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.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner

2020-03-24 Thread GitBox 
zabetak commented on a change in pull request #1869: [CALCITE-3868] Remove 
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r396993936
 
 

 ##
 File path: 
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
 ##
 @@ -392,35 +382,24 @@ public RelSet getSet(RelNode rel) {
 this.mapRel2Subset.clear();
 this.relImportances.clear();
 this.ruleQueue.clear();
-this.ruleNames.clear();
 this.materializations.clear();
 this.latticeByName.clear();
 this.provenanceMap.clear();
   }
 
   public List getRules() {
-return ImmutableList.copyOf(ruleSet);
+return ImmutableList.copyOf(mapDescToRule.values());
   }
 
   public boolean addRule(RelOptRule rule) {
 
 Review comment:
   If we choose to add this method to the superclass (instead of having 
`mapRuleDescription`) then here we can ovverride. 


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:
us...@infra.apache.org


With regards,
Apache Git Services