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 2019/12/25 04:02:32 UTC
[GitHub] [calcite] chunweilei opened a new pull request #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
chunweilei opened a new pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686
JIRA: https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-3630
----------------------------------------------------------------
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 #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361576836
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
> Assume that we enter the else branch and we create an empty LogicalValues. Then if the rule set does not contain a PhysicalValues we may end-up with a CannotPlanException.
This is not valid. Because without this change, `ReduceExpressionsRule` can also create an empty `LogicalValues`[1]. I agree that we should pay more attention when we use `setImportance`. But as I mentioned before, what this change does is to be consistent with what the if branch does[2].
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L199
[2] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L194-L216
----------------------------------------------------------------
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] zabetak commented on a change in pull request #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361596422
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
OK, I didn't see [1] before.
Other than maybe we could refactor slightly the code to keep the `setImportance` in one place (in [2]); leaving the 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] zabetak commented on a change in pull request #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361459282
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
Usages of `setImportance` indeed require some care. I think, this change for example could lead to a `CannotPlanException` that could be avoided before.
Assume that we enter the else branch and we create an empty `LogicalValues`. Then if the rule set does not contain a `PhysicalValues` we may end-up with a `CannotPlanException`. Without this change this wouldn't be the case since the old plan could still be matched by other rules and create a `PhysicalFilter` etc. I admit this is a contrived example but I wanted to highlight that setting importance explicitly might be risky.
----------------------------------------------------------------
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 #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361596696
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
IMHO, setImportance should be used as long as the new plan expression is better than old one, means just replace it. Even if there can be exception during planning, it is the fault of VolcanoPlanner, not the fault of `setImportance`. We should fix the VolcanoPlanner if the exception happens.
----------------------------------------------------------------
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 #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361576836
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
> Assume that we enter the else branch and we create an empty LogicalValues. Then if the rule set does not contain a PhysicalValues we may end-up with a CannotPlanException.
This is not valid. Because without this change, `ReduceExpressionsRule` can also create an empty `LogicalValues`[1]. I agree that we should pay more attention when we use `setImportance`. But as I mentioned before, what this change does is to be consistent with what the if branch does.
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L199
----------------------------------------------------------------
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 #1686: [CALCITE-3630]
Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686
----------------------------------------------------------------
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 #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361252973
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
Thanks for sharing, @danny0405 . IMO, if we already do it in this rule[1], it makes sense to do it to be consistent.
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L216
----------------------------------------------------------------
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 #1686:
[CALCITE-3630] Improve ReduceExpressionsRule
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1686: [CALCITE-3630] Improve ReduceExpressionsRule
URL: https://github.com/apache/calcite/pull/1686#discussion_r361251853
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -270,6 +270,8 @@ private void reduceNotNullableFilter(
} else {
call.transformTo(createEmptyRelOrEquivalent(call, filter));
}
+ // New plan is absolutely better than old plan.
+ call.getPlanner().setImportance(filter, 0.0);
Review comment:
`setImportance(filter, 0.0)` is not suggested to be used, i kind of remember see https://issues.apache.org/jira/browse/CALCITE-2223
----------------------------------------------------------------
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