[CALCITE-2204] Use BFS for propagating cost #643
Conversation
|
|
||
| void hierarchicalPropagateImprovement(VolcanoPlanner planner, RelMetadataQuery mq, | ||
| RelSubset subset, RelNode rel, Set<RelSubset> activeSet) { | ||
| Stack<Pair<RelNode, RelSubset>> propagateStack = new Stack<>(); |
There was a problem hiding this comment.
Stack class is very legacy, it extends Vector, maybe we can use some better util class
There was a problem hiding this comment.
ok, It only need Queue for FIFO instead of Stack
| @@ -319,9 +321,67 @@ RelNode buildCheapestPlan(VolcanoPlanner planner) { | |||
| */ | |||
| void propagateCostImprovements(VolcanoPlanner planner, RelMetadataQuery mq, | |||
| RelNode rel, Set<RelSubset> activeSet) { | |||
| // for (RelSubset subset : set.subsets) { | |||
LeoWangLZ
changed the title
|
@eolivelli It use ArrayDeque instead of Stack. Please review again. Thanks |
There was a problem hiding this comment.
Thank you for addressing my comments @LeoWangLZ for me is OK.
Unfortunately I have not enough information to say that this change is good, I did not run tests locally.
my +1 is non-binding, I did only a simple code review
|
@eolivelli Thanks again. I think Julian Hyde will review it |
|
could you add some test, say what scenario does this enhancement addresses, there are descriptions in your PR, but your implementation need to be guarded anyway. |
| @@ -360,6 +359,28 @@ void propagateCostImprovements0(VolcanoPlanner planner, RelMetadataQuery mq, | |||
| } | |||
| } | |||
|
|
|||
| void hierarchicalPropagateImprovement(VolcanoPlanner planner, RelMetadataQuery mq, | |||
There was a problem hiding this comment.
@LeoWangLZ what do you think about documenting BFS approach with examples?
|
I wonder how this plays with #552 which seems to update @LeoWangLZ , have you seen that? |
risdenk
changed the title
danny0405
force-pushed
the
master
branch
2 times, most recently
from
80f411d
to
ca27fe9
Compare
|
Close it because it has been fixed in e435954. |
In function propagateCostImprovements0, This subset is already in the chain being propagated to. This means that the graph is cyclic, and therefore the cost of this relational expression - not this subset - must be infinite. it may miss the best plan. I think we should change the way of propagation. now the algorithm like pre-order, it may use Breadth-first Search.
like
now the order of propagation is input->parent11->parent22->parent12.
use BFS, it maybe input->parent11->parent12->parent21->parent22.