[CALCITE-3630] Improve ReduceExpressionsRule - #1686
Conversation
| call.transformTo(createEmptyRelOrEquivalent(call, filter)); | ||
| } | ||
| // New plan is absolutely better than old plan. | ||
| call.getPlanner().setImportance(filter, 0.0); |
There was a problem hiding this comment.
setImportance(filter, 0.0) is not suggested to be used, i kind of remember see https://issues.apache.org/jira/browse/CALCITE-2223
There was a problem hiding this comment.
Thanks for sharing, @danny0405 . IMO, if we already do it in this rule[1], it makes sense to do it to be consistent.
There was a problem hiding this 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.
There was a problem hiding this 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].
There was a problem hiding this 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.
There was a problem hiding this 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.
4 participants
JIRA: https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-3630