You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by zuyu <gi...@git.apache.org> on 2017/08/24 22:22:58 UTC
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
GitHub user zuyu opened a pull request:
https://github.com/apache/incubator-quickstep/pull/281
Simplified the work order generation.
This PR revisited the work order generation process in the query execution engine, including removal of a recursive method `processOperator`. This method does not introduce much performance overside, while simplified the code base.
Note that there are two `QueryManagerSingleNode` unit tests deletion due to the impossibility in the real cases.
Assigned to @jianqiao, and CC'ed @hbdeshmukh.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zuyu/incubator-quickstep refactor-qe
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/281.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 #281
----
commit ee8d64440777207d9ab92d2d69257d97a0e187b3
Author: Zuyu Zhang <zu...@cs.wisc.edu>
Date: 2017-08-22T00:51:55Z
Simplified the work order generation.
----
---
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 #281: Simplified the work order generation.
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/281#discussion_r139330806
--- Diff: query_execution/tests/QueryManagerSingleNode_unittest.cpp ---
@@ -730,122 +731,4 @@ TEST_F(QueryManagerTest, TwoNodesDAGPartiallyFilledBlocksTest) {
EXPECT_TRUE(getOperatorFinishedStatus(id2));
}
-TEST_F(QueryManagerTest, MultipleNodesNoOutputTest) {
--- End diff --
I've fixed this bug. Please take another look.
After that, I will squash both commits into one.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/281#discussion_r137650750
--- Diff: query_execution/tests/QueryManagerSingleNode_unittest.cpp ---
@@ -730,122 +731,4 @@ TEST_F(QueryManagerTest, TwoNodesDAGPartiallyFilledBlocksTest) {
EXPECT_TRUE(getOperatorFinishedStatus(id2));
}
-TEST_F(QueryManagerTest, MultipleNodesNoOutputTest) {
--- End diff --
This is not a far fetched scenario. Steps to reproduce:
1. CREATE TABLE r (a INT, b INT);
2. CREATE TABLE s (c INT, d INT);
3. Select sum(a) from r, s where r.a = s.c;
---
[GitHub] incubator-quickstep issue #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/281
Looks good! Thanks for testing the cases.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/281
---
[GitHub] incubator-quickstep issue #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/281
I have finished one pass. Will take another look once the comments are addressed.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/281#discussion_r140540422
--- Diff: query_execution/QueryManagerBase.hpp ---
@@ -338,6 +285,19 @@ class QueryManagerBase {
**/
virtual bool checkRebuildOver(const dag_node_index index) const = 0;
+ /**
+ * @brief Check if the given operator has ever a normal work order.
+ *
+ * @param index The index of the given operator in the DAG.
+ *
+ * @return True if the operator has a normal work order, false otherwise.
+ **/
+ virtual bool hasEverNormalWorkOrders(const dag_node_index index) const = 0;
--- End diff --
Suggest to rename this function to ``hasEverProducedNormalWorkOrder`` to make more sense.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/281#discussion_r140544269
--- Diff: query_execution/WorkOrdersContainer.hpp ---
@@ -343,6 +346,18 @@ class WorkOrdersContainer {
getNumRebuildWorkOrders(operator_index);
}
+ /**
+ * @brief Check if the given operator has ever a normal work order.
+ *
+ * @param index The index of the given operator in the DAG.
+ *
+ * @return True if the operator has a normal work order, false otherwise.
+ **/
+ bool hasEverNormalWorkOrders(const std::size_t operator_index) const {
--- End diff --
Suggest to rename this function to ``hasEverProducedNormalWorkOrder`` to make more sense.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/281#discussion_r140544741
--- Diff: query_execution/tests/QueryManagerSingleNode_unittest.cpp ---
@@ -753,99 +752,51 @@ TEST_F(QueryManagerTest, MultipleNodesNoOutputTest) {
*
**/
for (QueryPlan::DAGNodeIndex i = 0; i < kNumNodes - 1; ++i) {
- query_plan_->addDirectDependency(ids[i + 1], ids[i], false);
- static_cast<MockOperator*>(query_plan_->getQueryPlanDAGMutable()->getNodePayloadMutable(ids[i]))
+ query_plan_->addDirectDependency(i + 1, i, false);
+ static_cast<MockOperator*>(query_plan_->getQueryPlanDAGMutable()->getNodePayloadMutable(i))
->setOutputRelationID(0xdead);
}
std::vector<const MockOperator*> operators;
for (QueryPlan::DAGNodeIndex i = 0; i < kNumNodes; ++i) {
- operators.push_back(static_cast<const MockOperator*>(&query_plan_->getQueryPlanDAG().getNodePayload(ids[i])));
+ operators.push_back(static_cast<const MockOperator*>(&query_plan_->getQueryPlanDAG().getNodePayload(i)));
}
constructQueryManager();
// operators[0] should have produced a workorder by now.
+ EXPECT_EQ(1, operators[0]->getNumCalls(MockOperator::kGetAllWorkOrders));
EXPECT_EQ(1, operators[0]->getNumWorkOrders());
unique_ptr<WorkerMessage> worker_message;
- worker_message.reset(query_manager_->getNextWorkerMessage(ids[0], -1));
+ worker_message.reset(query_manager_->getNextWorkerMessage(0, -1));
EXPECT_TRUE(worker_message != nullptr);
EXPECT_EQ(WorkerMessage::WorkerMessageType::kWorkOrder,
worker_message->getType());
- EXPECT_EQ(ids[0], worker_message->getRelationalOpIndex());
+ EXPECT_EQ(0, worker_message->getRelationalOpIndex());
delete worker_message->getWorkOrder();
- EXPECT_EQ(1, getNumWorkOrdersInExecution(ids[0]));
- EXPECT_FALSE(getOperatorFinishedStatus(ids[0]));
+ EXPECT_EQ(1, getNumWorkOrdersInExecution(0));
+ EXPECT_FALSE(getOperatorFinishedStatus(0));
- for (QueryPlan::DAGNodeIndex i = 0; i < kNumNodes; ++i) {
- EXPECT_EQ(1, operators[ids[i]]->getNumCalls(MockOperator::kGetAllWorkOrders));
+ for (QueryPlan::DAGNodeIndex i = 1; i < kNumNodes; ++i) {
+ EXPECT_EQ(0, operators[i]->getNumCalls(MockOperator::kGetAllWorkOrders));
}
// Send a message to QueryManager upon workorder (generated by operators[0])
// completion.
- EXPECT_TRUE(placeWorkOrderCompleteMessage(ids[0]));
+ EXPECT_TRUE(placeWorkOrderCompleteMessage(0));
for (QueryPlan::DAGNodeIndex i = 0; i < kNumNodes; ++i) {
- EXPECT_EQ(0, getNumWorkOrdersInExecution(ids[i]));
- EXPECT_TRUE(getOperatorFinishedStatus(ids[i]));
+ EXPECT_EQ(0, getNumWorkOrdersInExecution(i));
+ EXPECT_TRUE(getOperatorFinishedStatus(i));
if (i < kNumNodes - 1) {
EXPECT_EQ(1, operators[i + 1]->getNumCalls(MockOperator::kDoneFeedingInputBlocks));
}
}
}
-TEST_F(QueryManagerTest, OutOfOrderWorkOrderCompletionTest) {
--- End diff --
Why is this scenario not worthy to check?
---
[GitHub] incubator-quickstep issue #281: Simplified the work order generation.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/281
Hi @zuyu
The changes look good to me. Can you please try the following scenarios to make sure the scheduling works fine? These are some of the corner cases that I can think of:
1. Join of two empty tables.
2. Join of two empty tables with scan on one table.
3. Join of two empty tables with scan on both tables.
4. One empty table joined with another non-empty table.
5. One empty table joined with another non-empty table with a scan.
6. One empty table with scan joined with another non-empty table with a scan.
7. Any variation from above with an aggregation.
8. A non-equi join.
---
[GitHub] incubator-quickstep pull request #281: Simplified the work order generation.
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/281#discussion_r137808823
--- Diff: query_execution/tests/QueryManagerSingleNode_unittest.cpp ---
@@ -730,122 +731,4 @@ TEST_F(QueryManagerTest, TwoNodesDAGPartiallyFilledBlocksTest) {
EXPECT_TRUE(getOperatorFinishedStatus(id2));
}
-TEST_F(QueryManagerTest, MultipleNodesNoOutputTest) {
--- End diff --
Good point!
---