Plan clone refactor #1278
Plan clone refactor #1278
Conversation
src/execution_plan/execution_plan.h
Outdated
| @@ -34,7 +32,7 @@ struct ExecutionPlan { | |||
| * API for restructuring the op tree. | |||
| */ | |||
| /* Removes operation from execution plan. */ | |||
| void ExecutionPlan_RemoveOp(ExecutionPlan *plan, OpBase *op); | |||
| void ExecutionPlan_RemoveOp(OpBase *op); | |||
There was a problem hiding this comment.
We're so accustomed to pass the object we're working on, in this case both the plan and the op.
It feels a bit strange to have a function such as ExecutionPlan_RemoveOp which doesn't accepts an ExecutionPlan object,
although the implementation has very little to do with the plan I feel like an ASSERT(plan == op-plan) couldn't hurt and will justify restoring the original signature.
src/execution_plan/execution_plan.h
Outdated
| @@ -49,7 +47,7 @@ void ExecutionPlan_PushBelow(OpBase *a, OpBase *b); | |||
| void ExecutionPlan_NewRoot(OpBase *old_root, OpBase *new_root); | |||
|
|
|||
| /* Replace a with b. */ | |||
| void ExecutionPlan_ReplaceOp(ExecutionPlan *plan, OpBase *a, OpBase *b); | |||
| void ExecutionPlan_ReplaceOp(OpBase *a, OpBase *b); | |||
There was a problem hiding this comment.
Same comment as for ExecutionPlan_RemoveOp
src/execution_plan/execution_plan.c
Outdated
|
|
||
| static void _ExecutionPlan_FreeSubPlan(ExecutionPlan *plan) { | ||
| static void _ExecutionPlan_FreeSegment(ExecutionPlan *plan) { |
There was a problem hiding this comment.
As there are no longer Segments, I think renaming this to _ExecutionPlan_FreeInternals is more suitable.
src/execution_plan/execution_plan.c
Outdated
| _ExecutionPlan_FreeOperations(plan->root); | ||
| plan->root = NULL; | ||
| // Free an op tree and its associated ExecutionPlan segments. | ||
| static ExecutionPlan *_ExecutionPlan_Free(OpBase *op) { |
There was a problem hiding this comment.
Please rename to _ExecutionPlan_FreeOpTree or something similar
There was a problem hiding this comment.
It would be much nicer if this function would return void and we'll detect different "segments" when traversing
| void optimizePlan(ExecutionPlan *plan) { | ||
| /* Handle UNION of execution plans. */ | ||
| if(plan->is_union) { | ||
| uint segment_count = array_len(plan->segments); | ||
| for(uint i = 0; i < segment_count; i++) _optimizePlan(plan->segments[i]); | ||
| } else { | ||
| _optimizePlan(plan); | ||
| } | ||
| _optimizePlan(plan); | ||
| } |
There was a problem hiding this comment.
No need for a proxy function, rename _optimizePlan to optimizePlan
| */ | ||
| /* This function clones the input ExecutionPlan by recursively visiting its tree of ops. | ||
| * When an op is encountered that was constructed as part of a different ExecutionPlan segment, that segment | ||
| * and its internal members (FilterTree, record mapping, query graphs, and AST segment) are also cloned. */ | ||
| ExecutionPlan *ExecutionPlan_Clone(const ExecutionPlan *template) { | ||
| if(template == NULL) return NULL; |
There was a problem hiding this comment.
ASSERT(template != NULL) ?
| ExecutionPlan *ExecutionPlan_Clone(const ExecutionPlan *template) { | ||
| if(template == NULL) return NULL; | ||
| // Store the original AST pointer. | ||
| AST *master_ast = QueryCtx_GetAST(); | ||
| // Verify that the execution plan template is not prepared yet. | ||
| assert(template->prepared == false && "Execution plan cloning should be only on templates"); |
There was a problem hiding this comment.
Use debug assert, ASSERT
| // The "master" execution plan is the one constructed with the root op. | ||
| ExecutionPlan *clone = (ExecutionPlan *)clone_root->plan; | ||
| // Set the root op pointer. | ||
| clone->root = clone_root; |
There was a problem hiding this comment.
isn't clone_root->plan->root is clone_root ?
in which case we can return clone ?
Redundant.
There was a problem hiding this comment.
This turns out to not be redundant, the root pointer is never touched by the cloning or the New op routines.
| ExecutionPlan_AddOp(clone, cloned_child); | ||
| clone->record_map = raxClone(template->record_map); | ||
| if(template->ast_segment) clone->ast_segment = AST_ShallowCopy(template->ast_segment); | ||
| if(template->query_graph) clone->query_graph = QueryGraph_Clone(template->query_graph); |
There was a problem hiding this comment.
I think we can avoid both query_graph and connected_components if we introduce ref count to the QueryGraph object.
* WIP * Refactor ExecutionPlan_Free * Simplify RemoveOp signature * Fix UNION and optimization leaks * Address PR comments * Revert change to Remove and Replace signatures
Updates ExecutionPlan Clone and Free logic to crawl the op tree rather than storing ExecutionPlan segments. Removes extraneous members from the ExecutionPlan struct.
Resolves #1270, does not resolve #1183, which should be looked into further!