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<String, RelOptRule> mapDescToRule = new HashMap<>(); + protected final Map<String, RelOptRule> 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