Mailing-List: contact dev-help@quickstep.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@quickstep.incubator.apache.org
From: zuyu <git@git.apache.org>
To: dev@quickstep.incubator.apache.org
Reply-To: dev@quickstep.incubator.apache.org
References: <git-pr-45-incubator-quickstep@git.apache.org>
In-Reply-To: <git-pr-45-incubator-quickstep@git.apache.org>
Subject: [GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Content-Type: text/plain
Message-Id: <20160628214749.5E152E08FE@git1-us-west.apache.org>
Date: Tue, 28 Jun 2016 21:47:49 +0000 (UTC)
archived-at: Tue, 28 Jun 2016 21:47:54 -0000

Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/45#discussion_r68850154
  
    --- 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 --
    
    Then a quick fix is that add a true `pipeline_breaker` dependency from `window_aggregation` to `drop_merged_runs_index`.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---