You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by hbdeshmukh <gi...@git.apache.org> on 2016/08/23 16:51:26 UTC
[GitHub] incubator-quickstep pull request #96: QUICKSTEP-43 Introduced DestroyAggrega...
GitHub user hbdeshmukh opened a pull request:
https://github.com/apache/incubator-quickstep/pull/96
QUICKSTEP-43 Introduced DestroyAggregationState operator
- Similar to the DestroyHash operator, this operator destroys the AggregationState once the Finalize aggregation operator finishes its execution.
- Optimizer support for DestroyAggregationState operator.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep destroy-agg-state-operator
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/96.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 #96
----
----
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r76071306
--- Diff: relational_operators/WorkOrderFactory.cpp ---
@@ -420,22 +428,6 @@ WorkOrder* WorkOrderFactory::ReconstructFromProto(const serialization::WorkOrder
shiftboss_client_id,
bus);
}
- case serialization::WINDOW_AGGREGATION: {
--- End diff --
I forked this branch from the branch in #90, which in itself is quite old. Because of which some weird behavior may have happened. Thanks for pointing it out. I will remove any unrelated changes from this branch and let you know.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75954434
--- Diff: relational_operators/CMakeLists.txt ---
@@ -467,7 +481,6 @@ target_link_libraries(quickstep_relationaloperators_WorkOrderFactory
quickstep_relationaloperators_TableGeneratorOperator
quickstep_relationaloperators_TextScanOperator
quickstep_relationaloperators_UpdateOperator
- quickstep_relationaloperators_WindowAggregationOperator
--- End diff --
I was wondering why we removed this one?
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Hi @zuyu
I am not sure what's causing the failures. The check is similar to other methods related to aggregation state. I am not quite familiar with the distributed version tests, so if you can let me know why this check is not appropriate, that could be helpful.
On a side note, the QueryContext class lacks thread safety right now. For bigger query plans when several operators are under execution, there could be data races which can corrupt the class members.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
@hbdeshmukh Could you please explain more regarding parallelizing the `FinalizeAggregation` operator?
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Hi @hbdeshmukh That's fine. Then, after we remove the unused `releaseAggregationState` in `QueryContext`, I would merge this PR.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75955440
--- Diff: relational_operators/DestroyAggregationStateOperator.hpp ---
@@ -0,0 +1,118 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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_RELATIONAL_OPERATORS_DESTROY_AGGREGATION_STATE_OPERATOR_HPP_
+#define QUICKSTEP_RELATIONAL_OPERATORS_DESTROY_AGGREGATION_STATE_OPERATOR_HPP_
+
+#include <string>
+
+#include "query_execution/QueryContext.hpp"
+#include "relational_operators/RelationalOperator.hpp"
+#include "relational_operators/WorkOrder.hpp"
+#include "utility/Macros.hpp"
+
+#include "glog/logging.h"
+
+#include "tmb/id_typedefs.h"
+
+namespace tmb { class MessageBus; }
+
+namespace quickstep {
+
+class StorageManager;
+class WorkOrderProtosContainer;
+class WorkOrdersContainer;
+
+/** \addtogroup RelationalOperators
+ * @{
+ */
+
+/**
+ * @brief An operator which destroys a shared aggregation state.
+ **/
+class DestroyAggregationStateOperator : public RelationalOperator {
+ public:
+ /**
+ * @brief Constructor.
+ *
+ * @param query_id The ID of the query to which this operator belongs.
+ * @param aggr_state_index The index of the AggregationState in QueryContext.
+ **/
+ DestroyAggregationStateOperator(const std::size_t query_id,
+ const QueryContext::aggregation_state_id aggr_state_index)
+ : RelationalOperator(query_id),
+ aggr_state_index_(aggr_state_index),
+ work_generated_(false) {}
+
+ ~DestroyAggregationStateOperator() override {}
+
+ std::string getName() const override {
+ return "DestroyAggregationStateOperator";
+ }
+
+ bool getAllWorkOrders(WorkOrdersContainer *container,
+ QueryContext *query_context,
+ StorageManager *storage_manager,
+ const tmb::client_id scheduler_client_id,
+ tmb::MessageBus *bus) override;
+
+ bool getAllWorkOrderProtos(WorkOrderProtosContainer *container) override;
+
+ private:
+ const QueryContext::aggregation_state_id aggr_state_index_;
+ bool work_generated_;
+
+ DISALLOW_COPY_AND_ASSIGN(DestroyAggregationStateOperator);
+};
+
+/**
+ * @brief A WorkOrder produced by DestroyAggregationStateOperator.
+ **/
+class DestroyAggregationStateWorkOrder : public WorkOrder {
+ public:
+ /**
+ * @brief Constructor.
+ *
+ * @param query_id The ID of the query to which this WorkOrder belongs.
+ * @param aggr_state_index The index of the AggregationState in QueryContext.
+ * @param query_context The QueryContext to use.
+ **/
+ DestroyAggregationStateWorkOrder(const std::size_t query_id,
+ const QueryContext::aggregation_state_id aggr_state_index,
+ QueryContext *query_context)
--- End diff --
Code style: Please align w/ `Line 96`.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75955335
--- Diff: relational_operators/DestroyAggregationStateOperator.hpp ---
@@ -0,0 +1,118 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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_RELATIONAL_OPERATORS_DESTROY_AGGREGATION_STATE_OPERATOR_HPP_
+#define QUICKSTEP_RELATIONAL_OPERATORS_DESTROY_AGGREGATION_STATE_OPERATOR_HPP_
+
+#include <string>
+
+#include "query_execution/QueryContext.hpp"
+#include "relational_operators/RelationalOperator.hpp"
+#include "relational_operators/WorkOrder.hpp"
+#include "utility/Macros.hpp"
+
+#include "glog/logging.h"
+
+#include "tmb/id_typedefs.h"
+
+namespace tmb { class MessageBus; }
+
+namespace quickstep {
+
+class StorageManager;
+class WorkOrderProtosContainer;
+class WorkOrdersContainer;
+
+/** \addtogroup RelationalOperators
+ * @{
+ */
+
+/**
+ * @brief An operator which destroys a shared aggregation state.
+ **/
+class DestroyAggregationStateOperator : public RelationalOperator {
+ public:
+ /**
+ * @brief Constructor.
+ *
+ * @param query_id The ID of the query to which this operator belongs.
+ * @param aggr_state_index The index of the AggregationState in QueryContext.
+ **/
+ DestroyAggregationStateOperator(const std::size_t query_id,
+ const QueryContext::aggregation_state_id aggr_state_index)
--- End diff --
Code style: please align w/ the above line.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Hi @zuyu
Applied your suggestions. Thanks.
> As a side node for the new DestroyAggregationStateWorkOrder, we don't need this new method. Instead, using releaseAggregationState and the unique_ptr in the WorkOrder automatically destroys the content.
This change caused some of the tests (Aggregation operator unit test and execution generator - select, distinct and join) to fail. I didn't follow up on why they were failing and stuck to using ``destroyAggregationState`` instead.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Removed the ``releaseAggregationState`` method.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75956345
--- Diff: relational_operators/WorkOrderFactory.cpp ---
@@ -533,11 +530,13 @@ bool WorkOrderFactory::ProtoIsValid(const serialization::WorkOrder &proto,
return false;
}
+ const CatalogRelationSchema &build_relation = catalog_database.getRelationSchemaById(build_relation_id);
const CatalogRelationSchema &probe_relation = catalog_database.getRelationSchemaById(probe_relation_id);
for (int i = 0; i < proto.ExtensionSize(serialization::HashJoinWorkOrder::join_key_attributes); ++i) {
const attribute_id attr_id =
proto.GetExtension(serialization::HashJoinWorkOrder::join_key_attributes, i);
- if (!probe_relation.hasAttributeWithId(attr_id)) {
+ if (!build_relation.hasAttributeWithId(attr_id) ||
--- End diff --
Note that I had removed the check for `build_relation`, as the `attr_id` in `probe_relation` may be different to that in `build_relation`, although both refer to the same `CatalogAttribute`.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/96
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
From the initial observation, the reason of some query cases in the following unit tests failed seems that the aggregation state is already invalid before executing `DestroyAggregationStateWorkOrder`. I will take a further dig.
```
35 - quickstep_queryoptimizer_tests_distributed_executiongenerator_distinct (OTHER_FAULT)
37 - quickstep_queryoptimizer_tests_distributed_executiongenerator_index (OTHER_FAULT)
39 - quickstep_queryoptimizer_tests_distributed_executiongenerator_join (OTHER_FAULT)
40 - quickstep_queryoptimizer_tests_distributed_executiongenerator_select (OTHER_FAULT)
42 - quickstep_queryoptimizer_tests_distributed_executiongenerator_tablegenerator (OTHER_FAULT)
```
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75954125
--- Diff: query_execution/QueryContext.hpp ---
@@ -184,7 +184,7 @@ class QueryContext {
/**
* @brief Release the given AggregationOperationState.
*
- * @param id The id of the AggregationOperationState to destroy.
+ * @param id The id of the AggregationOperationState to release.
--- End diff --
I think we could remove this method, as it will not be used any more.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Sure. Right now we are using a single threaded implementation for the FinalizeAggregation operator. It does two things -
1. Merging the multiple aggregation hash tables in one single global hash table. Note that same group by key can be present in different hash tables, hence the merging step is necessary.
2. Finalizing the aggregate value. e.g. in case of AVG, we delay the computation of actual average until very last. Until that point we keep the running sum and count.
There are multiple ways in which the FinalizeAggregation can be parallelized. One of them is divide the aggregation hash tables such that one hash table belongs to a given partition. We partition an input tuple using the group by key attributes and route that tuple to the appropriate hash table. Now, there are no overlapping keys across any two hash tables, hence the step 1 mentioned above is not necessary. As step 2 naturally relies on the output of 1, it can be done independently for each hash table. Therefore 1 and 2 can be parallelized.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r77213683
--- Diff: query_execution/QueryContext.hpp ---
@@ -184,7 +184,7 @@ class QueryContext {
/**
* @brief Release the given AggregationOperationState.
*
- * @param id The id of the AggregationOperationState to destroy.
+ * @param id The id of the AggregationOperationState to release.
--- End diff --
It's getting used in WorkOrderFactory.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Some of the copyright blocks have been changed, I am fixing them now.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
I'd suggest we close this PR and then look at the distributed version tests. Those were added in a hurry if I remember correctly (and without a PR). So how about @zuyu work on the distributed version fixes in a later PR.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
@zuyu I described the motivation for this operator in the JIRA issue, I will rephrase it here.
>
Background: We use a class called AggregationState that manages the aggregation related data structures during the execution of a query. At present this class is destroyed through the FinalizeAggregation work order, which is executed by a single thread.
>
In the future, we would like to parallelize the FinalizeAggregation operator. Therefore for the destruction of the AggregationState, we need for a separate operator. This design pattern is similar to what is used in HashJoins, where we have a separate DestroyHashOperator.
>
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Hi @zuyu
Thanks for the comments. I have addressed them.
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
@hbdeshmukh Could you please give me the intuition of adding this new operator? Is it related to partition-aware aggregation? Thanks!
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r77553384
--- Diff: query_execution/QueryContext.hpp ---
@@ -184,7 +184,7 @@ class QueryContext {
/**
* @brief Release the given AggregationOperationState.
*
- * @param id The id of the AggregationOperationState to destroy.
+ * @param id The id of the AggregationOperationState to release.
--- End diff --
@hbdeshmukh Since we changed the semantics of `FinalizeAggregationOperator`, when constructing `FinalizeAggregationWorkOrder`, [`WorkOrderFactory`](https://github.com/apache/incubator-quickstep/pull/96/files#diff-2c12385b34f03b5eb61a3f5df00505eeR157) should also use the get version instead. And it fixes the distributed unit tests. Otherwise, it would also cause memory leaks for the distributed version.
@pateljm The test failures are related to the missed changes in new feature for the distributed version, not my existing code. The main components in the distributed query execution engine was code reviewed, although not the test drivers (but the most driver code is forked from the existing codebase).
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r77553580
--- Diff: query_execution/QueryContext.hpp ---
@@ -195,6 +195,17 @@ class QueryContext {
}
/**
+ * @brief Destroy the given aggregation state.
+ *
+ * @param id The ID of the AggregationOperationState to destroy.
+ **/
+ inline void destroyAggregationState(const aggregation_state_id id) {
--- End diff --
As a side node for the new `DestroyAggregationStateWorkOrder`, we don't need this new method. Instead, using `releaseAggregationState` and the `unique_ptr` in the `WorkOrder` automatically destroys the content.
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75955675
--- Diff: relational_operators/WorkOrderFactory.cpp ---
@@ -420,22 +428,6 @@ WorkOrder* WorkOrderFactory::ReconstructFromProto(const serialization::WorkOrder
shiftboss_client_id,
bus);
}
- case serialization::WINDOW_AGGREGATION: {
--- End diff --
Again, why deleting `WindowAggr` code?
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
Is it related to the `hash table pool`?
---
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 issue #96: QUICKSTEP-43 Introduced DestroyAggregationSta...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/96
@hbdeshmukh If we build w/ the distributed version enabled using `-DENABLE_DISTRIBUTED=true`, we will observe multiple distributed-version-related unit tests failures due to sanity checks on [the Aggregation State](https://github.com/apache/incubator-quickstep/blob/destroy-agg-state-operator/query_execution/QueryContext.hpp#L204).
I would like to merge this PR after all the tests pass. I am looking into this issue, but you may also want to investigate on this. Thanks!
---
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 #96: QUICKSTEP-43 Introduced DestroyAggrega...
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/96#discussion_r75956515
--- Diff: relational_operators/WorkOrder.proto ---
@@ -42,6 +42,7 @@ enum WorkOrderType {
TEXT_SCAN = 18;
UPDATE = 19;
WINDOW_AGGREGATION = 20;
+ DESTROY_AGGREGATION_STATE = 21;
--- End diff --
Please add a `TODO` to reorder the number based on the alphabet order.
---
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.
---