[GitHub] [calcite] zabetak commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…

[GitBox <gi...@apache.org>]

zabetak commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r521986581



##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -197,6 +198,10 @@ public boolean isRuleExcluded(RelOptRule rule) {
     return this;
   }
 
+  @Override public void addExtraMaterializationRules(List<UnifyRule> rules) {

Review comment:
       I also share @jcamachor view point. If we decide to allow customization of this part its better to allow full-control. 
   
   Furthermore, to be consistent with the existing APIs of the planner, I would opt for a per rule addition:
   `addMaterializationRule(UnifyRule rule)`
   
   Going one step further, is it really necessary to introduce a new API for this? The point is that we want to add new rules to the planner so can't we use the existing `addRule(RelOptRule)` interface? I guess we want to distinguish those rules that are specific to materialization but this could be done in the implementation of `addRule`.
   
   What do you think?




----------------------------------------------------------------
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