Github user shixuan-fan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68819187
--- Diff: query_optimizer/ExecutionGenerator.cpp ---
@@ -1575,21 +1579,12 @@ void ExecutionGenerator::convertSort(const
P::SortPtr &physical_sort) {
merged_runs_destination_proto->set_relational_op_index(merge_run_operator_index);
sorted_output_destination_proto->set_relational_op_index(merge_run_operator_index);
- // Do not add merged_runs_relation into 'temporary_relation_info_vec_'
- // and create the DropTableOperator for it at the end. Instead, add the
drop
- // operator right here, because the relation won't be used by any other
operator.
- const QueryPlan::DAGNodeIndex drop_merged_runs_index =
- execution_plan_->addRelationalOperator(
- new DropTableOperator(
- query_handle_->query_id(),
- *merged_runs_relation,
- optimizer_context_->catalog_database(),
- false /* only_drop_blocks */));
- execution_plan_->addDirectDependency(
- drop_merged_runs_index,
- merge_run_operator_index,
- true /* is_pipeline_breaker */);
-
+ // REVIEW(Shixuan): There might be an issue with the original code: if a
throw
+ // happens before the plan is executed, merged_runs_relation will not be
dropped
+ // by dropAllTemporaryRelations() if not added to
temporary_relation_info_vec,
+ // which might cause "RelationNameCollision".
+ temporary_relation_info_vec_.emplace_back(merge_run_operator_index,
--- End diff --
You are right. We should drop the table as early as possible. I previously
thought the "DropTable" operator is inserted right after each temp_relation
node. My bad.
However, When I tried to catch the physical plan for WindowAggregate using
the old code, the first time it is okay, however, the second time it will
report "RelationNameCollision". I believe the reason is that this temp relation
is not dropped by dropAllTemporaryRelations() in the "try-catch".
To handle the above issue, we need to add a check for WindowAggregate
before visiting the children of each physical node, in order to avoid the
creation of this temp relation (merged_runs_relation), as it will not be
dropped after `THROW_SQL_ERROR()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
