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/03/10 12:55:50 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #1853: [CALCITE-3851]
Store nodes with zero importance with a set
liyafan82 opened a new pull request #1853: [CALCITE-3851] Store nodes with zero importance with a set
URL: https://github.com/apache/calcite/pull/1853
Currently, volcano planner stores rel node importances with a map. In fact, the value of the map can only be 0.
So there is no need to store the values, and we replace the map with a set to store nodes whose importances are 0.
This makes the code logic clearer, and avoids instability caused by comparing floating point values.
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r393451365
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPlanner.java
##########
@@ -307,9 +307,24 @@ RelNode register(
*
* @param rel Relational expression
* @param importance Importance
+ *
+ * @deprecated This API will be removed in a future release, as we no longer
+ * support the concept of node importance. Please use
+ * {@link RelOptPlanner#pruneNode(RelNode)} method instead.
Review comment:
needs update
----------------------------------------------------------------
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 #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r394090678
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -265,7 +264,7 @@ private void checkDuplicateSubsets(Deque<RelSubset> subsets,
* for a particular
* {@link VolcanoPlannerPhase phase of the planner's execution}.
*/
- private static class PhaseMatchList {
+ static class PhaseMatchList {
/**
Review comment:
I don't think it is a good practice to modify it just for the test case. Can we change the test case?
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r392455605
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -221,9 +221,13 @@ public long getRelMetadataTimestamp(RelNode rel) {
return 0;
}
+ @Deprecated
public void setImportance(RelNode rel, double importance) {
}
+ public void pruneNode(RelNode rel) {
+ }
Review comment:
Is it possible to add a API to `RelNode`? e.g.
```
void RelNode.setPruned();
boolean RelNode.isPruned();
```
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r392570817
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -1003,9 +999,8 @@ private void dumpSets(PrintWriter pw) {
}
}
}
- Double importance = relImportances.get(rel);
- if (importance != null) {
- pw.print(", importance=" + importance);
+ if (prunedNodes.contains(rel)) {
+ pw.print(", importance=0");
Review comment:
Revised. Thank 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1853: [CALCITE-3851]
Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#issuecomment-607010616
> Could you please resolve the conflicts?
Resolved. Thanks for your kind reminder.
----------------------------------------------------------------
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 #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r392516215
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -221,9 +221,13 @@ public long getRelMetadataTimestamp(RelNode rel) {
return 0;
}
+ @Deprecated
public void setImportance(RelNode rel, double importance) {
}
+ public void pruneNode(RelNode rel) {
+ }
Review comment:
I suggest just name it "prune", since it's pretty clear that you are going to prune a RelNode by looking at the parameter. It's also consistent with the original naming (setImportance() not setNodeImportance()).
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r394294185
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -265,7 +264,7 @@ private void checkDuplicateSubsets(Deque<RelSubset> subsets,
* for a particular
* {@link VolcanoPlannerPhase phase of the planner's execution}.
*/
- private static class PhaseMatchList {
+ static class PhaseMatchList {
/**
Review comment:
Sure. Sounds reasonable. I have revised the test case, and restored 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
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r395458048
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPlanner.java
##########
@@ -307,9 +307,24 @@ RelNode register(
*
* @param rel Relational expression
* @param importance Importance
+ *
+ * @deprecated This API will be removed in a future release, as we no longer
+ * support the concept of node importance. Please use
+ * {@link RelOptPlanner#prune(RelNode)} method instead.
*/
+ @Deprecated
Review comment:
Revised. Thank 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Store nodes with zero importance with a set
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Store nodes with zero importance with a set
URL: https://github.com/apache/calcite/pull/1853#discussion_r391385082
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -135,16 +135,16 @@
new IdentityHashMap<>();
/**
- * The importance of relational expressions.
+ * The nodes with zero importance.
*
- * <p>The map contains only RelNodes whose importance has been overridden
+ * <p>The set contains only RelNodes whose importance has been set to 0
* using {@link RelOptPlanner#setImportance(RelNode, double)}. Other
* RelNodes are presumed to have 'normal' importance.
*
* <p>If a RelNode has 0 importance, all {@link RelOptRuleCall}s using it
* are ignored, and future RelOptRuleCalls are not queued up.
*/
- final Map<RelNode, Double> relImportances = new HashMap<>();
+ final Set<RelNode> nodesToSkip = new HashSet<>();
Review comment:
Sounds good. Revised accordingly.
----------------------------------------------------------------
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 #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r393429871
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -265,7 +264,7 @@ private void checkDuplicateSubsets(Deque<RelSubset> subsets,
* for a particular
* {@link VolcanoPlannerPhase phase of the planner's execution}.
*/
- private static class PhaseMatchList {
+ static class PhaseMatchList {
/**
Review comment:
Why 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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r395447976
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPlanner.java
##########
@@ -307,9 +307,24 @@ RelNode register(
*
* @param rel Relational expression
* @param importance Importance
+ *
+ * @deprecated This API will be removed in a future release, as we no longer
+ * support the concept of node importance. Please use
+ * {@link RelOptPlanner#prune(RelNode)} method instead.
*/
+ @Deprecated
Review comment:
Missing deprecation version.
----------------------------------------------------------------
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 #1853: [CALCITE-3851]
Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
chunweilei commented on issue #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#issuecomment-606995997
Could you please resolve the conflicts?
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r392570807
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -221,9 +221,13 @@ public long getRelMetadataTimestamp(RelNode rel) {
return 0;
}
+ @Deprecated
public void setImportance(RelNode rel, double importance) {
}
+ public void pruneNode(RelNode rel) {
+ }
Review comment:
@xndai Revised. Thanks for your good suggestion.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r394064261
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -265,7 +264,7 @@ private void checkDuplicateSubsets(Deque<RelSubset> subsets,
* for a particular
* {@link VolcanoPlannerPhase phase of the planner's execution}.
*/
- private static class PhaseMatchList {
+ static class PhaseMatchList {
/**
Review comment:
This class is referenced in a test case.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Store nodes with zero importance with a set
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Store nodes with zero importance with a set
URL: https://github.com/apache/calcite/pull/1853#discussion_r391384985
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -917,13 +917,13 @@ RelNode changeTraitsUsingConverters(
public void setImportance(RelNode rel, double importance) {
assert rel != null;
if (importance == 0d) {
- relImportances.put(rel, importance);
+ nodesToSkip.add(rel);
}
}
Double getImportance(RelNode rel) {
assert rel != null;
- return relImportances.get(rel);
+ return nodesToSkip.contains(rel) ? 0d : null;
}
Review comment:
Removed. Thanks for the good suggestion.
----------------------------------------------------------------
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 #1853: [CALCITE-3851]
Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1853:
[CALCITE-3851] Store nodes with zero importance with a set
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1853: [CALCITE-3851] Store nodes with zero importance with a set
URL: https://github.com/apache/calcite/pull/1853#discussion_r390495050
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -135,16 +135,16 @@
new IdentityHashMap<>();
/**
- * The importance of relational expressions.
+ * The nodes with zero importance.
*
- * <p>The map contains only RelNodes whose importance has been overridden
+ * <p>The set contains only RelNodes whose importance has been set to 0
* using {@link RelOptPlanner#setImportance(RelNode, double)}. Other
* RelNodes are presumed to have 'normal' importance.
*
* <p>If a RelNode has 0 importance, all {@link RelOptRuleCall}s using it
* are ignored, and future RelOptRuleCalls are not queued up.
*/
- final Map<RelNode, Double> relImportances = new HashMap<>();
+ final Set<RelNode> nodesToSkip = new HashSet<>();
Review comment:
prunedNodes
----------------------------------------------------------------
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] danny0405 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r395447875
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -221,9 +221,13 @@ public long getRelMetadataTimestamp(RelNode rel) {
return 0;
}
+ @Deprecated
public void setImportance(RelNode rel, double importance) {
Review comment:
Mark a deprecation version here
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r392457444
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -1003,9 +999,8 @@ private void dumpSets(PrintWriter pw) {
}
}
}
- Double importance = relImportances.get(rel);
- if (importance != null) {
- pw.print(", importance=" + importance);
+ if (prunedNodes.contains(rel)) {
+ pw.print(", importance=0");
Review comment:
change to `pruned`
----------------------------------------------------------------
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] liyafan82 commented on issue #1853: [CALCITE-3851]
Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#issuecomment-600383573
> Could you please change the title and squash commits?
Done. Thanks for your kind reminder.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r395457917
##########
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##########
@@ -221,9 +221,13 @@ public long getRelMetadataTimestamp(RelNode rel) {
return 0;
}
+ @Deprecated
public void setImportance(RelNode rel, double importance) {
Review comment:
Marked. Thanks for your kind reminder.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #1853:
[CALCITE-3851] Replace the node importance map with a set for pruned nodes
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1853: [CALCITE-3851] Replace the node importance map with a set for pruned nodes
URL: https://github.com/apache/calcite/pull/1853#discussion_r394064320
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPlanner.java
##########
@@ -307,9 +307,24 @@ RelNode register(
*
* @param rel Relational expression
* @param importance Importance
+ *
+ * @deprecated This API will be removed in a future release, as we no longer
+ * support the concept of node importance. Please use
+ * {@link RelOptPlanner#pruneNode(RelNode)} method instead.
Review comment:
Nice catch. Thank 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1853:
[CALCITE-3851] Store nodes with zero importance with a set
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1853: [CALCITE-3851] Store nodes with zero importance with a set
URL: https://github.com/apache/calcite/pull/1853#discussion_r390494901
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -917,13 +917,13 @@ RelNode changeTraitsUsingConverters(
public void setImportance(RelNode rel, double importance) {
assert rel != null;
if (importance == 0d) {
- relImportances.put(rel, importance);
+ nodesToSkip.add(rel);
}
}
Double getImportance(RelNode rel) {
assert rel != null;
- return relImportances.get(rel);
+ return nodesToSkip.contains(rel) ? 0d : null;
}
Review comment:
You can remove 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:
users@infra.apache.org
With regards,
Apache Git Services