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/08/03 15:12:46 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #2094: [CALCITE-3409] Add an interface in MaterializedViewSubstitutionVisito…
xy2953396112 opened a new pull request #2094:
URL: https://github.com/apache/calcite/pull/2094
…r to allow registering UnifyRule (XuZhaoHui JinXing)
----------------------------------------------------------------
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
[GitHub] [calcite] jcamachor commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r714094628
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -214,7 +232,11 @@
hepPlanner.setRoot(root);
root = hepPlanner.findBestExp();
- return new SubstitutionVisitor(target, root).go(materialization.tableRel);
+ return new SubstitutionVisitor(target, root, ImmutableList.
+ <SubstitutionVisitor.UnifyRule>builder()
+ .addAll(materializationRules)
+ .addAll(SubstitutionVisitor.DEFAULT_RULES)
Review comment:
This seems to be a new change. As @wojustme pointed out, this seems to prevent anyone from using their own materialization rules without including the default ones. Can we remove this line? Same result should be obtained by passing the default rules as part of the externally injected ones.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wojustme commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713841077
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -214,7 +232,11 @@
hepPlanner.setRoot(root);
root = hepPlanner.findBestExp();
- return new SubstitutionVisitor(target, root).go(materialization.tableRel);
+ return new SubstitutionVisitor(target, root, ImmutableList.
+ <SubstitutionVisitor.UnifyRule>builder()
+ .addAll(materializationRules)
+ .addAll(SubstitutionVisitor.DEFAULT_RULES)
Review comment:
So, Real running rules of unify is collection of SubstitutionVisitor's default and external-injected?
Will we meet one case, which it only want work with external-injected rules, not included default?
This case may occur, for example: we overload some `SqlAggFunction` to replace calcite default `SqlAggFunction`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor closed pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
jcamachor closed pull request #2094:
URL: https://github.com/apache/calcite/pull/2094
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713852598
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -214,7 +232,11 @@
hepPlanner.setRoot(root);
root = hepPlanner.findBestExp();
- return new SubstitutionVisitor(target, root).go(materialization.tableRel);
+ return new SubstitutionVisitor(target, root, ImmutableList.
+ <SubstitutionVisitor.UnifyRule>builder()
+ .addAll(materializationRules)
+ .addAll(SubstitutionVisitor.DEFAULT_RULES)
Review comment:
Your example is specialized and does not apply to `Calcite`.
In `Calcite`, it should be extended on the default rules.
If you have special needs, external rules can also be registered.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r469787716
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
Maybe we can be compatible with the original interface.In my project, this is how to register external materialized view matching rules.
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -279,6 +282,10 @@ public RelNode getRoot() {
return ImmutableList.copyOf(materializations);
}
+ @Override public void registerMaterializationRules(List<UnifyRule> rules) {
+ materializationRules = ImmutableList.copyOf(rules);
+ }
+
Review comment:
ok,make sense.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1741,7 +1741,7 @@ private static int fieldCnt(MutableRel rel) {
}
/** Explain filtering condition and projections from MutableCalc. */
- private static Pair<RexNode, List<RexNode>> explainCalc(MutableCalc calc) {
+ public static Pair<RexNode, List<RexNode>> explainCalc(MutableCalc calc) {
final RexShuttle shuttle = getExpandShuttle(calc.program);
final RexNode condition = shuttle.apply(calc.program.getCondition());
Review comment:
This modifier is used in `org.apache.calcite.test.MaterializationTest.CustomizedMaterializationRule`.
If external rules need to use this method, we should change it.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r524892936
##########
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:
@jcamachor @zabetak
Thank you very much. I have update the code. The rule of materialized view is `AbstractUnifyRule`, not `RelOptRule`. I suggest registering materialized view recognition algorithm through `addMaterializationRules(List<UnifyRule> rules)`. This interface can also be compatible with `UnifyRule`. If a single 'unifyrule' is registered, it is ok. In the project, we call ` org.apache.calcite . plan.RelOptMaterializations#useMaterializedViews () ' interface for view recognition. Hope to get feedback, thank you again.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 edited a comment on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-925454999
@wojustme @hsyuan @jcamachor
Thanks for review.Please review again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516873161
##########
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:
Yes, I understand that they are quite general. My point was that since we are already changing the interface, we could use the same method to add any materialization rules, allowing full control over which ones are enabled throughout the planning. However, I am fine with this approach if others think it is fine.
----------------------------------------------------------------
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
[GitHub] [calcite] zabetak commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r712232285
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +66,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
Review comment:
There is no javadoc for the new parameter.
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -202,6 +204,11 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Experimental
Review comment:
I think we have switched to using apiguardian for this kind of annoations.
`@API(since = "1.28", status = API.Status.EXPERIMENTAL)`
Moreover, I think the annotation should be placed in the interface (`RelOptPlanner`) and not in the implementation class.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r468972294
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1741,7 +1741,7 @@ private static int fieldCnt(MutableRel rel) {
}
/** Explain filtering condition and projections from MutableCalc. */
- private static Pair<RexNode, List<RexNode>> explainCalc(MutableCalc calc) {
+ public static Pair<RexNode, List<RexNode>> explainCalc(MutableCalc calc) {
final RexShuttle shuttle = getExpandShuttle(calc.program);
final RexNode condition = shuttle.apply(calc.program.getCondition());
Review comment:
Do we have to change the modifier?
----------------------------------------------------------------
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
[GitHub] [calcite] zabetak commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by 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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r524892936
##########
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:
@jcamachor @zabetak
Thank you very much. I have update the code. The rule of materialized view is `AbstractUnifyRule`, not `RelOptRule`. I suggest registering materialized view recognition algorithm through `addMaterializationRules(List<UnifyRule> rules)`. This interface can also be compatible with `UnifyRule`. If a single `UnifyRule` is registered, it is ok. In the project, we call `org.apache.calcite . plan.RelOptMaterializations#useMaterializedViews ()` interface for view recognition. Hope to get feedback, thank you again.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713622530
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -202,6 +204,11 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Experimental
Review comment:
According to comments, avoid changing the `RelOptPlanner` APIs. so we ignore `@Experimental` annoation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713624649
##########
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:
@hsyuan
I have updated the code.Avoid changing the `RelOptPlanner` APIs,and It can be compatible with the original implementation.Please help review.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r471878147
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
If we have to make this change, let us deprecate it first.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-672465993
> @hsyuan @chunweilei
> Please help review this pr when you have time.Thanks a lot.
OK. Will review it this week.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r468965053
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
Oops, this is a breaking change.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516391375
##########
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:
The materialized recognition rules currently implemented are quite general (based on pure relational algebra). I think adding some additional rules for the materialized recognition of specific scenes will help us to solve the materialized recognition of specific scenes, rather than adding them all together. Even if no additional rules are added, the current physical and chemical recognition rules, such as `CalcToCalcUnifyRule` and `JoinOnCalcsToJoinUnifyRule`, are still the most basic materialized recognition rules, and we should keep them unchanged.I think it's better to retain the most basic materialized recognition rules and add custom materialized recognition rules at the same time.
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -50,6 +50,12 @@
*/
public abstract class RelOptMaterializations {
+ @Deprecated // to be removed before 2.0
+ public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
+ final RelNode rel, List<RelOptMaterialization> materializations) {
+ return useMaterializedViews(rel, materializations, null);
Review comment:
Thanks, update the code.
----------------------------------------------------------------
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
[GitHub] [calcite] jcamachor commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516184828
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -50,6 +50,12 @@
*/
public abstract class RelOptMaterializations {
+ @Deprecated // to be removed before 2.0
+ public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
+ final RelNode rel, List<RelOptMaterialization> materializations) {
+ return useMaterializedViews(rel, materializations, null);
Review comment:
Should the third parameter be an empty list rather than null (I think otherwise we may hit an NPE)?
##########
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:
Instead of having a mechanism to add _extra_ materialization rules, have we thought about adding all materialization rules through this mechanism? That way we can actually customize the full behavior rather than having a subset of internally defined rules that are hardcoded.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r711318688
##########
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:
@jcamachor @zabetak
@xy2953396112 has updated the code according to the comments.
Any more comments?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r479627601
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
Should we keep the default implementation without custom materialized recognition rules? Method overloading.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r510861444
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
@chunweilei Can we be compatible with existing interfaces?
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r524892936
##########
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:
@jcamachor @zabetak
Thank you very much. I have update the code. The rule of materialized view is `AbstractUnifyRule`, not `RelOptRule`. I suggest registering materialized view recognition algorithm through `addMaterializationRules(List<UnifyRule> rules)`. This interface can also be compatible with `UnifyRule`. If a single 'unifyrule' is registered, it is ok. In the project, we call `org.apache.calcite . plan.RelOptMaterializations#useMaterializedViews ()` interface for view recognition. Hope to get feedback, thank you again.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r472031071
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +60,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
final List<RelOptMaterialization> applicableMaterializations =
getApplicableMaterializations(rel, materializations);
Review comment:
ok, update it.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r468971222
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -279,6 +282,10 @@ public RelNode getRoot() {
return ImmutableList.copyOf(materializations);
}
+ @Override public void registerMaterializationRules(List<UnifyRule> rules) {
+ materializationRules = ImmutableList.copyOf(rules);
+ }
+
Review comment:
This interface is a little confusing. Because even user does not call this method, normal rules are still be added. Maybe `addExtraMaterializationRules`?
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r468971222
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -279,6 +282,10 @@ public RelNode getRoot() {
return ImmutableList.copyOf(materializations);
}
+ @Override public void registerMaterializationRules(List<UnifyRule> rules) {
+ materializationRules = ImmutableList.copyOf(rules);
+ }
+
Review comment:
This interface is a little confusing. It seems user has to call the method to enable rules. But actually even user does not call this method, normal rules are still be added. Maybe change the name to `addExtraMaterializationRules`?
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713621369
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##########
@@ -60,7 +66,8 @@
* materialized views used in the transformation.
*/
public static List<Pair<RelNode, List<RelOptMaterialization>>> useMaterializedViews(
- final RelNode rel, List<RelOptMaterialization> materializations) {
+ final RelNode rel, List<RelOptMaterialization> materializations,
+ List<SubstitutionVisitor.UnifyRule> materializationRules) {
Review comment:
ok, 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r713623719
##########
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:
@zabetak
Thank you for spending a long time on this pr.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-925454999
@wojustme @jcamachor
Thanks for review.Please review again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 edited a comment on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-925454999
@wojustme @jcamachor
Thanks for review.Please review again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 edited a comment on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-925454999
@wojustme @Jesus Camacho Rodriguez @jcamachor
Thanks for review.Please review again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r712234684
##########
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:
@hsyuan I added a few comments in the JIRA but don't have much time to follow up on this. I would be happy if someone else can take over.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#issuecomment-670271879
@hsyuan @chunweilei
Please help review this pr when you have time.Thanks a lot.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r525100348
##########
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:
@jcamachor @zabetak
Similar to this PR, register custom materialized view normalization algorithm code[1]. Please help review when you have time. Thanks.
[1] [Add an interface in RelOptMaterializations to allow registering normalization rules](https://github.com/apache/calcite/pull/2262)
----------------------------------------------------------------
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