[CALCITE-6462] VolcanoPlanner internal valid may throw exception when… #3851
Conversation
| @@ -982,11 +982,17 @@ void propagateCostImprovements(RelNode rel) { | |||
| if (!relNode.getTraitSet().satisfies(subset.getTraitSet())) { | |||
| continue; | |||
| } | |||
| if (!cost.isLt(subset.bestCost)) { | |||
|
|
|||
| // Update subset best and best's cost when we find a cheaper rel | |||
There was a problem hiding this comment.
these two comments do not really seem to describe what the code next to them is doing
There was a problem hiding this comment.
This is the condition when we need to update the best and best's cost.
There was a problem hiding this comment.
The line commented does not update anything, it just calls continue.
The comment should say the opposite: if the cost is higher, continue.
NobiGo
added
the
LGTM-will-merge-soon
| // best's cost is changed | ||
|
|
||
| // Update the current best's cost when we find cost is changed | ||
| if (relNode == subset.best && cost.equals(subset.bestCost)) { |
There was a problem hiding this comment.
A question, why relNode == subset.best, but the cost is different?
There was a problem hiding this comment.
Hi @YiwenWu , This is because RelSet's one of the subset find a cheaper rel's cost, We need to update the parents the best rel and best cost. In theory, this cost will become smaller, But according to the debug info, The SQL I added in the JdbcAdapterTest shows same rel node 's cost sometimes will become bigger. So I update it.
There was a problem hiding this comment.
Maybe you could add this information in a comment?
There was a problem hiding this comment.
@YiwenWu @mihaibudiu Thanks for the review. I have added the comment, please review it. Thanks.
There was a problem hiding this comment.
In general I would avoid using "I" in comments.
I am not sure what "the same RelNode's" means here.
Can you make sure that the comment is grammatically correct?
For example, ", We" is not right, but that's not the only problem.
There was a problem hiding this comment.
@mihaibudiu Hi, Thanks for your reminder. I update the Java doc. Hopefully, there is no ambiguity now.
There was a problem hiding this comment.
First, this is not JavaDoc, it's a regular comment.
Please make the entire comment as simple as possible.
I still don't understand what the comment says and how it relates to the code. Please refer in the comment to variables in the code by name. This comparison is for equality, but the comments talks about "smaller". I don't understand how the comment describes the code. No need to make a story about "I" or "we", just say what the code does.
There was a problem hiding this comment.
@mihaibudiu , To my knowledge, Java documentation and Java annotation are the same as code comments. So maybe as you say comment is more eloquently here. And if we just describe the code code does, I will change the doc to :
The Best RelNode's cost can be changed in propagate progress, as shown by the test {@link JdbcAdapterTest.testVolcanoPlannerInternalValid}
There was a problem hiding this comment.
@mihaibudiu Hi , I think the comment is good now, So if you think still need to improve, Please submit another PR to improve it. I am ok with this.
There was a problem hiding this comment.
@NobiGo I'd be happy to test this PR in detail and then submit a PR modification comment
NobiGo
removed
the
LGTM-will-merge-soon
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
Outdated
Show resolved
Hide resolved
… log trace is enabled
|
… log trace is enabled