You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by shixuan-fan <gi...@git.apache.org> on 2016/06/28 16:31:56 UTC
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
GitHub user shixuan-fan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/45
QUICKSTEP-20: PhysicalGenerator support for Window Aggregation
- The PhysicalGenerator could now generate a physical plan for window aggregate function using OneToOne strategy.
- The ExecutionGenerator will catch window aggregate function and throw an exception.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep SQL-window-aggregation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/45.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #45
----
commit ef6ad916374e30f5694d046a5d398cfb07de6d5d
Author: shixuan-fan <sh...@apache.org>
Date: 2016-06-28T16:25:52Z
PhysicalGenerator support for Window Aggregation
----
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68812537
--- Diff: query_optimizer/physical/WindowAggregate.hpp ---
@@ -0,0 +1,133 @@
+/**
+ * Copyright 2011-2015 Quickstep Technologies LLC.
+ * Copyright 2015 Pivotal Software, Inc.
+ * Copyright 2016, Quickstep Research Group, Computer Sciences Department,
+ * University of Wisconsin\u2014Madison.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+#ifndef QUICKSTEP_QUERY_OPTIMIZER_PHYSICAL_WINDOW_AGGREGATE_HPP_
+#define QUICKSTEP_QUERY_OPTIMIZER_PHYSICAL_WINDOW_AGGREGATE_HPP_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "query_optimizer/OptimizerTree.hpp"
+#include "query_optimizer/expressions/Alias.hpp"
+#include "query_optimizer/expressions/AttributeReference.hpp"
+#include "query_optimizer/expressions/ExpressionUtil.hpp"
+#include "query_optimizer/expressions/NamedExpression.hpp"
+#include "query_optimizer/physical/Physical.hpp"
+#include "query_optimizer/physical/PhysicalType.hpp"
+#include "utility/Macros.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+namespace optimizer {
+namespace physical {
+
+/** \addtogroup OptimizerLogical
+ * @{
+ */
+
+class WindowAggregate;
+typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
+
+/**
+ * @brief Window Aggregate operator that computes window aggregate expressions.
+ */
+class WindowAggregate : public Physical {
+ public:
+ PhysicalType getPhysicalType() const override {
+ return PhysicalType::kWindowAggregate;
+ }
+
+ std::string getName() const override { return "WindowAggregate"; }
+
+ /**
+ * @return The input physical node.
+ */
+ const PhysicalPtr input() const { return input_; }
+
+ /**
+ * @return Window aggregate expression.
+ */
+ inline const expressions::AliasPtr window_aggregate_expression() const {
+ return window_aggregate_expression_;
+ }
+
+ PhysicalPtr copyWithNewChildren(
+ const std::vector<PhysicalPtr> &new_children) const override {
+ DCHECK_EQ(getNumChildren(), new_children.size());
+ return Create(new_children[0], window_aggregate_expression_);
+ }
+
+ std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
+
+ std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
+
+ bool maybeCopyWithPrunedExpressions(
+ const expressions::UnorderedNamedExpressionSet &referenced_expressions,
+ PhysicalPtr *output) const override {
+ // The project expressions for an WindowAggregate cannot be changed.
+ return false;
+ }
+
+ /**
+ * @brief Creates an WindowAggregate physical node.
+ *
+ * @param input The input node.
+ * @param window_aggregate_expression The window aggregate expression.
+ * @return An immutable WindowAggregate node.
+ */
+ static WindowAggregatePtr Create(
+ PhysicalPtr input,
--- End diff --
As we discussed [before](https://github.com/apache/incubator-quickstep/pull/39#discussion_r68329405), please use a `const` reference for `shared_ptr`.
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by shixuan-fan <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68811789
--- 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 --
@shixuan-fan I prefer the old way that deletes the tmp sorted relation eagerly, as the resultant blocks belonged to that relation would be huge.
No need to worry about the relation name collision, as it is created per query basis.
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68846898
--- 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 --
I think the actual executing order of `DropTable` depends on the scheduler based on a topological order. But in the optimizer generated DAG, a `sort` operator typically is placed as the last one, so in most case the new way is similar to the old way.
I suggest to debug the query w/ `window aggregation` by printing out the DAG and the operator finish order (hack in `query_execution/QueryManager`).
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/45
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by zuyu <gi...@git.apache.org>.
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68814000
--- Diff: query_optimizer/physical/WindowAggregate.hpp ---
@@ -0,0 +1,133 @@
+/**
+ * Copyright 2011-2015 Quickstep Technologies LLC.
+ * Copyright 2015 Pivotal Software, Inc.
+ * Copyright 2016, Quickstep Research Group, Computer Sciences Department,
+ * University of Wisconsin\u2014Madison.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+#ifndef QUICKSTEP_QUERY_OPTIMIZER_PHYSICAL_WINDOW_AGGREGATE_HPP_
+#define QUICKSTEP_QUERY_OPTIMIZER_PHYSICAL_WINDOW_AGGREGATE_HPP_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "query_optimizer/OptimizerTree.hpp"
+#include "query_optimizer/expressions/Alias.hpp"
+#include "query_optimizer/expressions/AttributeReference.hpp"
+#include "query_optimizer/expressions/ExpressionUtil.hpp"
+#include "query_optimizer/expressions/NamedExpression.hpp"
+#include "query_optimizer/physical/Physical.hpp"
+#include "query_optimizer/physical/PhysicalType.hpp"
+#include "utility/Macros.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+namespace optimizer {
+namespace physical {
+
+/** \addtogroup OptimizerLogical
+ * @{
+ */
+
+class WindowAggregate;
+typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
+
+/**
+ * @brief Window Aggregate operator that computes window aggregate expressions.
+ */
+class WindowAggregate : public Physical {
+ public:
+ PhysicalType getPhysicalType() const override {
+ return PhysicalType::kWindowAggregate;
+ }
+
+ std::string getName() const override { return "WindowAggregate"; }
+
+ /**
+ * @return The input physical node.
+ */
+ const PhysicalPtr input() const { return input_; }
+
+ /**
+ * @return Window aggregate expression.
+ */
+ inline const expressions::AliasPtr window_aggregate_expression() const {
+ return window_aggregate_expression_;
+ }
+
+ PhysicalPtr copyWithNewChildren(
+ const std::vector<PhysicalPtr> &new_children) const override {
+ DCHECK_EQ(getNumChildren(), new_children.size());
+ return Create(new_children[0], window_aggregate_expression_);
+ }
+
+ std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
+
+ std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
+
+ bool maybeCopyWithPrunedExpressions(
+ const expressions::UnorderedNamedExpressionSet &referenced_expressions,
+ PhysicalPtr *output) const override {
+ // The project expressions for an WindowAggregate cannot be changed.
+ return false;
+ }
+
+ /**
+ * @brief Creates an WindowAggregate physical node.
+ *
+ * @param input The input node.
+ * @param window_aggregate_expression The window aggregate expression.
+ * @return An immutable WindowAggregate node.
+ */
+ static WindowAggregatePtr Create(
+ PhysicalPtr input,
+ const expressions::AliasPtr &window_aggregate_expression) {
+ return WindowAggregatePtr(new WindowAggregate(input,
+ window_aggregate_expression));
+ }
+
+ protected:
+ void getFieldStringItems(
+ std::vector<std::string> *inline_field_names,
+ std::vector<std::string> *inline_field_values,
+ std::vector<std::string> *non_container_child_field_names,
+ std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
+ std::vector<std::string> *container_child_field_names,
+ std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const override;
+
+ private:
+ WindowAggregate(
+ PhysicalPtr input,
--- End diff --
Ditto for the const reference to a `shared_ptr`.
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68849181
--- 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 --
In the `window aggregation`, the window aggregation operator will be placed after the sort operator, that is, it will do the actual calculation after the table is sorted. So the work flow would be:
input_table -> sort -> window_aggregation -> output.
In my current implementation, there will be an exception at the physical node with `kWindowAggregate` type, which is preceded by a sort operator, and that's why `RelationNameCollision` happened.
---
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.
---
[GitHub] incubator-quickstep pull request #45: QUICKSTEP-20: PhysicalGenerator suppor...
Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/45#discussion_r68853383
--- 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 --
Sounds good. Will revert to the original code. Thanks for all the discussion :)
---
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.
---