Re: [DISCUSS] Retire Quickstep podling from Apache Incubator
+1 to retire. Cheers, Zuyu
[GitHub] incubator-quickstep issue #362: Add virtual destructors where needed to remo...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/362 LGTM. Squash the commits and once the checks pass, I can merge. ---
[GitHub] incubator-quickstep pull request #362: Add virtual destructors where needed ...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/362#discussion_r224539118 --- Diff: third_party/src/tmb/tests/message_bus_unittest_common.h --- @@ -205,7 +207,7 @@ class ConnectorThread : public Thread { } protected: - void Run() { + void Run() override { --- End diff -- Add `override` destructors in this file. ---
[GitHub] incubator-quickstep pull request #362: Add virtual destructors where needed ...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/362#discussion_r224538733 --- Diff: utility/lip_filter/SingleIdentityHashFilter.hpp --- @@ -66,6 +66,8 @@ class SingleIdentityHashFilter : public LIPFilter { DCHECK_GE(filter_cardinality, 1u); } + ~SingleIdentityHashFilter() {}; --- End diff -- Add `override`. ---
[GitHub] incubator-quickstep issue #362: Add virtual destructors where needed to remo...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/362 For completeness, please add `override` for the destructors of derived classes of `GeneratorFunctionHandle`, `LIPFilter`, and `BaseClass`. Thanks! ---
[GitHub] incubator-quickstep pull request #361: Optimized the common case in pure-mem...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/361 Optimized the common case in pure-memory TMB. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep tmb-opt Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/361.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 #361 commit d9e3a2af9927075bb5ad43c23a2a42f11fcbf1cd Author: Zuyu Zhang Date: 2018-06-28T20:42:36Z Optimized the common case in pure-memory TMB. ---
[GitHub] incubator-quickstep pull request #359: Fixed the build issues regarding tmb ...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/359 Fixed the build issues regarding tmb benchmark. This minor PR fixed some build issues related to the tmb benchmark directory. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep tmb-bench-build Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/359.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 #359 commit e36fc7497fb63cadff7bcb3e30f42a3c94a087e2 Author: Zuyu Zhang Date: 2018-06-19T01:18:26Z Fixed the build issues regarding tmb benchmark. ---
[GitHub] incubator-quickstep issue #358: Fix a bug in HashJoinOperator
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/358 Hold for adding tests to cover these buggy cases to prevent future bugs. ---
[GitHub] incubator-quickstep pull request #355: QUICKSTEP-127 Data provider thread
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/355#discussion_r192827208 --- Diff: storage/CMakeLists.txt --- @@ -120,6 +120,11 @@ configure_file ( QS_PROTOBUF_GENERATE_CPP(storage_AggregationOperationState_proto_srcs storage_AggregationOperationState_proto_hdrs AggregationOperationState.proto) +if (ENABLE_NETWORK_CLI) +QS_PROTOBUF_GENERATE_CPP(storage_BlockWire_proto_srcs --- End diff -- Two whitespace indentation, please. Similarly below. ---
[GitHub] incubator-quickstep pull request #357: Fixed the command execution bug in th...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/357 Fixed the command execution bug in the distributed version. This PR fixed the command execution bugs in the distributed version, and now it is consistent with the single-node version. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep dist-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/357.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 #357 commit 89ddcb48b32f352659600cdcdc55fe059184a06a Author: Zuyu Zhang Date: 2018-05-21T21:39:42Z Fixed the command execution bug in the distributed version. ---
[GitHub] incubator-quickstep pull request #338: Fixed the gRPC Problem for Data Excha...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/338#discussion_r189711855 --- Diff: storage/StorageManager.cpp --- @@ -687,8 +687,13 @@ StorageManager::BlockHandle StorageManager::loadBlockOrBlob( #ifdef QUICKSTEP_DISTRIBUTED const string domain_network_address = getPeerDomainNetworkAddress(BlockIdUtil::Domain(block)); DLOG(INFO) << "Pulling Block " << BlockIdUtil::ToString(block) << " from " << domain_network_address; + +// Customize the grpc channel +grpc::ChannelArguments channelArgs; +channelArgs.SetMaxReceiveMessageSize(kGrpcChanelSize); --- End diff -- Please add a condition wrapper based on the `GRPC` version number for this API call, since not every version has this API. ---
[GitHub] incubator-quickstep pull request #356: DO NOT MERGE: Added the grpc in Travi...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/356 DO NOT MERGE: Added the grpc in Travis CI. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep travis-grpc-new Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/356.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 #356 commit d6554af49ae003e7999f4367369dc3e6242ad18b Author: Zuyu Zhang Date: 2018-05-21T18:42:15Z Added the grpc in Travis CI. ---
[GitHub] incubator-quickstep issue #355: QUICKSTEP-127 Data provider thread
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/355 We do not test the distributed version in CI due to previous time-out issue while installing `grpc` by source code. ---
[GitHub] incubator-quickstep pull request #355: QUICKSTEP-127 Data provider thread
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/355#discussion_r189664042 --- Diff: storage/DataProviderThread.hpp --- @@ -0,0 +1,103 @@ +/** + * 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_STORAGE_DATA_PROVIDER_THREAD_HPP_ +#define QUICKSTEP_STORAGE_DATA_PROVIDER_THREAD_HPP_ + +#include + +#include "query_execution/QueryExecutionTypedefs.hpp" +#include "storage/StorageConfig.h" +#include "threading/Thread.hpp" +#include "utility/Macros.hpp" + +#include "tmb/native_net_client_message_bus.h" + +namespace quickstep { + +class CatalogDatabase; +class CatalogRelation; +class QueryProcessor; +class StorageManager; + +/** \addtogroup Storage + * @{ + */ + +/** + * @brief A thread that provides access to query results (e.g. storage blocks), + *to Quickstep clients. --- End diff -- Could you please provide more info regarding this class, including which class will own this class, and why it needs write permissions to both `CatalogDatabase` and `QueryProcessor`? Thanks! ---
[GitHub] incubator-quickstep pull request #354: Fixed the union-all elimiation case w...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/354 Fixed the union-all elimiation case where some project expression is not an AttributeReference. This PR fixed for the case where the only one child of the union-all query is non-empty, and the child has a non-AttributeReference project expression: ``` SELECT 1 AS a FROM not_empty UNION ALL SELECT 0 AS a FROM empty; ``` You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep eliminate-empty-node-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/354.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 #354 commit f7d1132672f4ef6399dbbe77541f93911c2d640b Author: Zuyu Zhang Date: 2018-05-08T04:58:35Z Fixed the union-all elimiation case where some project expression is not an AttributeReference. ---
[GitHub] incubator-quickstep pull request #353: Minor bug fixes and refactors.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/353 Minor bug fixes and refactors. This PR contains most work from @jianqiao when working on the datalog branch, including bug fix and some refactors. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep bug-fix-refactor Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/353.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 #353 commit 7aa91392e67cdb96a75e36522f243c71330ab811 Author: Zuyu Zhang Date: 2018-05-07T21:08:35Z Minor bug fixes and refactors. ---
[GitHub] incubator-quickstep pull request #351: Use Exactness info in Catalog stats.
Github user zuyu closed the pull request at: https://github.com/apache/incubator-quickstep/pull/351 ---
[GitHub] incubator-quickstep pull request #352: QUICKSTEP-125: Fixed the non-determin...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/352 QUICKSTEP-125: Fixed the non-determinism in JoinReordering. This PR fixed the bug in join reordering due to the pointer address-based comparison. It also added a new rule that prefers the build side which does not produce output, and did minor refactors for the file. Assigned to @jianqiao. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep join-reordering-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/352.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 #352 commit 439c64afa195be3c067b6889917a64ce48ec27c3 Author: Zuyu Zhang Date: 2018-05-05T19:21:53Z QUICKSTEP-125: Fixed the non-determinism in JoinReordering due to pointer cmp. ---
[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/300#discussion_r186265132 --- Diff: query_optimizer/ExecutionGenerator.cpp --- @@ -966,7 +975,9 @@ void ExecutionGenerator::convertHashJoin(const P::HashJoinPtr &physical_plan) { const CatalogRelation *probe_relation = probe_relation_info->relation; // FIXME(quickstep-team): Add support for self-join. - if (build_relation == probe_relation) { + // We check to see if the build_predicate is null as certain queries that + // support hash-select fuse will result in the first check being true. + if (build_relation == probe_relation && physical_plan->build_predicate() == nullptr) { --- End diff -- This change introduces a bug, because the execution engine uses the relation id to determine the build or the probe side. In the self-join case, it thus always goes to the if-case (mostly the build side), even we actually reference the probe side with the same relation id. We do not need any further action for this, because #347 will introduce the self-join. ---
[GitHub] incubator-quickstep pull request #351: Use Exactness info in Catalog stats.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/351 Use Exactness info in Catalog stats. When we get stats info, we should not ignore the `exactness` flag. This PR checks the flag before answer whether a stat exists. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep use-exactness Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/351.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 #351 commit cf37cf046d77ca98c19fab82c67ab8181a09d1ca Author: Zuyu Zhang Date: 2018-05-05T15:24:37Z Use Exactness info in Catalog stats. ---
[GitHub] incubator-quickstep pull request #350: Fixed the bug regarding EliminateEmpt...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/350 Fixed the bug regarding EliminateEmptyNode and InsertSelection. This PR fixed a bug regarding `EliminateEmptyNode` and `InsertSelection` that could be easily reproduced as the following: ``` CREATE TABLE temp (a int); \analyze INSERT INTO temp SELECT i FROM generate_series(1, 100) AS gs(i); ``` The problem is that `InsertSelection` allows the destination table to be empty, but the rule does not catch that. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep elimination-rule-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/350.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 #350 commit 77287a78890284139273dd5344e7d9b141f7ee25 Author: Zuyu Zhang Date: 2018-05-04T18:04:02Z Fixed the bug regarding EliminateEmptyNode and InsertSelection. ---
[GitHub] incubator-quickstep pull request #318: DO NOT MERGE: Optimizing Hash reparti...
Github user zuyu closed the pull request at: https://github.com/apache/incubator-quickstep/pull/318 ---
[GitHub] incubator-quickstep pull request #349: Fixed the bug regarding EliminateEmpt...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/349 Fixed the bug regarding EliminateEmptyNode and Analyze command. This PR fixed a bug introduced in #342. The problem was that that rule does not distinguish between an analyze command and an aggregate query. Thus, both reduces into a `SELECT` query on a temp table. Ironically, `quickstep_cli_tests_commandexecutor_analyze` does not proof the correctness of the rule because `\analyze` has no effect under the rule and no stats added. In other words, we could easily reproduce the bug: ``` CREATE TABLE r (a int); \analyze r SELECT COUNT(*) FROM r; // this produce nothing (in 0 row), instead of a ZERO (in 1 row). ``` This fix adds a flag for `\analyze` command, and also transforms an aggregate query correctly. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep elimination-rule-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/349.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 #349 commit 8cd618cd5d8d2ea2a7383fb9d4be65227807b60d Author: Zuyu Zhang Date: 2018-05-03T01:24:01Z Fixed the bug regarding EliminateEmptyNode and Analyze command. ---
[GitHub] incubator-quickstep issue #347: QUICKSTEP-121: Added the self-join support.
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/347 Holds util PR #348 merges. ---
[GitHub] incubator-quickstep pull request #348: Refactored ScalarCaseExpression.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/348 Refactored ScalarCaseExpression. This PR does the following: 1. Moved the static methods into the cpp file. 1. Added a fast path for `getAllValuesForJoin`, similar to other existing methods. 1. Added more unit tests to cover the join cases with a static branch. 1. Removed a unnecessary overridden virtual function `getRelationIdForValueAccessor`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep case-expr-opt Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/348.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 #348 commit dc4b5ddedb11077ae6a6a3a92a841a42c4a8b819 Author: Zuyu Zhang Date: 2018-05-02T21:06:59Z Refactored ScalarCaseExpression. ---
[GitHub] incubator-quickstep pull request #347: DO NOT MERGE: QUICKSTEP-121: Added th...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/347 DO NOT MERGE: QUICKSTEP-121: Added the self-join support. This PR added the self-join support by assigning the join side info, instead of relying on the relation id. Note that now `NestedLoopsJoin` has to use the same term as `HashJoin` w.r.t. join side. In the other words, the left side is the `probe`, while the right is the `build`. Also, the join result pair is in the format of ``. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep self-join Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/347.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 #347 commit 4055d65a207b63775f2cf4c4841b33da15d70cd2 Author: Zuyu Zhang Date: 2018-04-30T03:50:02Z Added the self-join support. ---
[GitHub] incubator-quickstep issue #346: QUICKSTEP-124: Add a python script to auto f...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/346 LGTM! ---
[GitHub] incubator-quickstep issue #343: Fix all CMakeLists.txt for automated process...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/343 LGTM! ---
[GitHub] incubator-quickstep pull request #345: Minor code style fix.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/345 Minor code style fix. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep code-style-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/345.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 #345 commit 68f006e487b251901d118d26912b4d58dd3e93e1 Author: Zuyu Zhang Date: 2018-04-27T02:39:18Z Code style fix. ---
[GitHub] incubator-quickstep pull request #344: Quickstep-123: Fixed the missing 'has...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/344 Quickstep-123: Fixed the missing 'has_repartition' in FilterJoin. Added the missing `has_repartition` in `FilterJoin`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep filter-join-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/344.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 #344 commit 1772b8a1cd9664c9553a566efdafb97ddd427c57 Author: Zuyu Zhang Date: 2018-04-27T02:20:01Z Fixed the missing 'has_repartition' in FilterJoin. ---
[GitHub] incubator-quickstep pull request #342: DO NOT MERGE: Quickstep-119: Added th...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/342 DO NOT MERGE: Quickstep-119: Added the rule that eliminates a HashJoin to a Selection if possible. This PR adds an optimization rule that eliminates a join if at least one side is empty. TODO: add unit tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep simplify-join-rule Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/342.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 #342 commit c670a5bec9374f9c613461114c234eeea25f2de1 Author: Zuyu Zhang Date: 2018-04-18T22:15:34Z Added the rule that eliminates a HashJoin to a Selection if possible. ---
[GitHub] incubator-quickstep issue #341: Used MergeFrom instead of CopyFrom.
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/341 `CopyFrom` = `Clear` + `MergeFrom`. Since we set a value for each new `proto`, we do not need `Clear` to reset the memory. ---
[GitHub] incubator-quickstep pull request #341: Used MergeFrom instead of CopyFrom.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/341 Used MergeFrom instead of CopyFrom. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep use-merge-from Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/341.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 #341 commit 612e103cb590a8f7735c5e7af7529c4c68956bbf Author: Zuyu Zhang Date: 2018-04-20T20:46:50Z Used MergeFrom instead of CopyFrom. ---
[GitHub] incubator-quickstep issue #338: Fixed the gRPC Problem for Data Exchange
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/338 LGTM! ---
[GitHub] incubator-quickstep issue #336: Fixed the bug that Executor / Cli does not c...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/336 Distributing the tbl files to every data node is a better approach for large data files. On the other hand, `InsertTuples` has a lot overhead by attribute type checks and tuple copy costs. ---
[GitHub] incubator-quickstep issue #336: Fixed the bug that Executor / Cli does not c...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/336 @yuanchenl #337 should avoid the segfault. But the root cause of the segfault is that we do not have a distributed FS for the tbl files. A workaround is that copying all tbl files to every node except for `cli`. On the other hand, to test any new features, we need to build `Quickstep` again inside `Docker` container. Is this what you do? ---
[GitHub] incubator-quickstep pull request #337: Check File Handle in TextScanWorkOrde...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/337 Check File Handle in TextScanWorkOrder to avoid segfault. This PR avoids the segfault mentioned [here](https://github.com/apache/incubator-quickstep/pull/336#issuecomment-370674172) when opening an input file that does not exist. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep fp-check Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/337.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 #337 commit 934f3ab87b642fa6230a951f051cd44828b96f57 Author: Zuyu Zhang Date: 2018-03-06T09:00:10Z Check File Handle in TextScanWorkOrder to avoid segfault. ---
[GitHub] incubator-quickstep pull request #336: Fixed the bug that Executor / Cli doe...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/336 Fixed the bug that Executor / Cli does not create directory for StorageManager. Assigned to @jianqiao. @yuanchenl Please test this PR using Docker. Thanks! You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep dist-directory Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/336.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 #336 commit 9fdb0db4bfd616433d02aeeebe1e162e6040a7e7 Author: Zuyu Zhang Date: 2018-03-01T03:03:46Z Fixed the bug that Executor / Cli does not create directory for StorageManager. ---
[GitHub] incubator-quickstep issue #335: Dockerfile Support for Distributed Deploymen...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/335 Before merging this PR, please rebase this branch with the master. ---
[GitHub] incubator-quickstep issue #334: Fix iwyu include path
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/334 Merged. ---
[GitHub] incubator-quickstep issue #335: Dockerfile Support for Distributed Deploymen...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/335 @yuanchenl Please review and minimize your changes (i.e, using a stable version of `cmake` instead of `rc` version, why installing `grpc` twice (once using `apt-get` and built from source code), and so does `protobuf`). Does this PR achieve the goal that by starting a container specified by the modified Dockerfile, `quickstep_cli_shell` is running as a server to accept queries from some input interface (say browser)? ---
[GitHub] incubator-quickstep issue #332: Small adjustments in star schema cost model ...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/332 LGTM! ---
[GitHub] incubator-quickstep issue #333: Fix SeparateChainingHashTable::resize()
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/333 LGTM! ---
[GitHub] incubator-quickstep pull request #332: Small adjustments in star schema cost...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/332#discussion_r167114902 --- Diff: query_optimizer/cost_model/StarSchemaSimpleCostModel.cpp --- @@ -493,7 +493,7 @@ std::size_t StarSchemaSimpleCostModel::getNumDistinctValues( return stat.getNumDistinctValues(rel_attr_id); } } - return estimateCardinalityForTableReference(table_reference); + return estimateCardinalityForTableReference(table_reference) * 0.1; --- End diff -- Could you please explain more regarding why `0.1` would be a good guess for the number of distinct values? ---
[GitHub] incubator-quickstep issue #331: Add a cmake option to handle the Travis CI t...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/331 LGTM! Cool finding! ---
[GitHub] incubator-quickstep issue #327: QUICKSTEP-113 Remove glog source code from t...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/327 Hi @hbdeshmukh, This PR that uses `glog 0.3.5` has a side effect that does not catch before: [it requires `cmake` version `3.0`](https://github.com/google/glog/blob/v0.3.5/CMakeLists.txt#L1), but the default `cmake` version in `Ubuntu 14.04 LTS` is `2.8.12.2`. ---
[GitHub] incubator-quickstep pull request #328: QUICKSTEP-114 Upgrade cpplint
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/328#discussion_r158369912 --- Diff: parser/ParseString.hpp --- @@ -115,4 +115,4 @@ class ParseString : public ParseTreeNode { } // namespace quickstep -#endif /* QUICKSTEP_PARSER_PARSE_STRING_HPP_ */ +#endif /* QUICKSTEP_PARSER_PARSE_STRING_HPP_ */ --- End diff -- Suggest to `// QUICKSTEP_PARSER_PARSE_STRING_HPP_`. ---
[GitHub] incubator-quickstep pull request #328: QUICKSTEP-114 Upgrade cpplint
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/328#discussion_r158370165 --- Diff: storage/BasicColumnStoreValueAccessor.hpp --- @@ -108,25 +109,25 @@ class BasicColumnStoreValueAccessorHelper { } template - inline const void* getAttributeValue(const tuple_id tuple, + inline const void* getAttributeValue(const tuple_id input_tuple, --- End diff -- To be consistent with above, how about using `tid`? ---
[GitHub] incubator-quickstep issue #327: QUICKSTEP-113 Remove glog source code from t...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/327 It works on `Mac`, but I have not tested using `XCode`. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019903 --- Diff: transaction/AdmissionControl.hpp --- @@ -0,0 +1,83 @@ +/** + * 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_TRANSACTION_ADMISSION_CONTROL_HPP_ +#define QUICKSTEP_TRANSACTION_ADMISSION_CONTROL_HPP_ + +#include "utility/Macros.hpp" +#include "transaction/TransactionTable.hpp" + +namespace quickstep { +namespace transaction { + +class AdmissionControl { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + AdmissionControl(TransactionTable *transaction_table) {} + + virtual ~AdmissionControl() {} + + /** + * @brief Admit a transaction to the system. + * + * @note Check the transaction's compatibility with the other running + * transactions. If it is compatible, let it run, otherwise put + * the transaction in the waiting list. + * + * @note Accesses to the transaction_table may need protection. + * + * @param tid The ID of the given transaction. + * @param resource_requests A vector of pairs such that each pair has a + *resource ID and its requested access mode. + * @return True if the transaction can be admitted, false if it has to wait. + */ + virtual bool admitTransaction(const transaction_id tid, +const std::vector> &resource_requests) { +return false; + } + + /** + * @brief Attempt to admit a waiting transaction. + * + * @note Check the transaction's compatibility with the other running + * transactions. If it is compatible, let it run, otherwise put + * the transaction in the waiting list. + * + * @note Accesses to the transaction_table may need protection. + * + * @param tid The ID of the given transaction. + * @return True if the transaction can be admitted, false if the + * transaction has to wait. + */ + virtual bool admitWaitingTransaction(const transaction_id tid) { +return false; + } --- End diff -- Optionally, I think it is better to mark these two APIs as `pure virtual` methods. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019919 --- Diff: transaction/AdmissionControl.hpp --- @@ -0,0 +1,83 @@ +/** + * 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_TRANSACTION_ADMISSION_CONTROL_HPP_ +#define QUICKSTEP_TRANSACTION_ADMISSION_CONTROL_HPP_ + +#include "utility/Macros.hpp" +#include "transaction/TransactionTable.hpp" + +namespace quickstep { +namespace transaction { + +class AdmissionControl { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + AdmissionControl(TransactionTable *transaction_table) {} + + virtual ~AdmissionControl() {} + + /** + * @brief Admit a transaction to the system. + * + * @note Check the transaction's compatibility with the other running + * transactions. If it is compatible, let it run, otherwise put + * the transaction in the waiting list. + * + * @note Accesses to the transaction_table may need protection. + * + * @param tid The ID of the given transaction. + * @param resource_requests A vector of pairs such that each pair has a + *resource ID and its requested access mode. + * @return True if the transaction can be admitted, false if it has to wait. + */ + virtual bool admitTransaction(const transaction_id tid, +const std::vector> &resource_requests) { +return false; + } + + /** + * @brief Attempt to admit a waiting transaction. + * + * @note Check the transaction's compatibility with the other running + * transactions. If it is compatible, let it run, otherwise put + * the transaction in the waiting list. + * + * @note Accesses to the transaction_table may need protection. + * + * @param tid The ID of the given transaction. + * @return True if the transaction can be admitted, false if the + * transaction has to wait. + */ + virtual bool admitWaitingTransaction(const transaction_id tid) { +return false; + } + + private: + DISALLOW_COPY_AND_ASSIGN(AdmissionControl); +}; + +} // namespace transaction +} // namespace quickstep + +#endif //QUICKSTEP_TRANSACTION_ADMISSION_CONTROL_HPP_ --- End diff -- Add a whitespace after `//`. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019967 --- Diff: transaction/CMakeLists.txt --- @@ -125,8 +138,8 @@ target_link_libraries(quickstep_transaction quickstep_transaction_ResourceId quickstep_transaction_StronglyConnectedComponents quickstep_transaction_Transaction + quickstep_transaction_CompatibilityChecker quickstep_transaction_TransactionTable) - --- End diff -- Revert this change. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153020128 --- Diff: transaction/CompatibilityChecker.hpp --- @@ -0,0 +1,106 @@ +/** + * 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_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ +#define QUICKSTEP_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ + +#include +#include + +#include "transaction/TransactionTable.hpp" +#include "utility/Macros.hpp" + +namespace quickstep { + +class CatalogRelation; + +namespace transaction { + +/** + * @brief A class that checks the compatibility of a transaction with the + *other running transactions. + */ +class CompatibilityChecker { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + CompatibilityChecker(TransactionTable *transaction_table) {} + + virtual ~CompatibilityChecker() {} + + /** + * @brief Check if the given transaction is compatible with other + *running transactions. + * @note The transaction table has the list of all the running or pending + * transactions. Lookup the table and check if the given transaction + * is compatible, based on the CC protocol specifications. + * @note Accesses to the transaction_table may need protection. + + * @param tid The ID of the given transaction + * @param resource_requests A vector of pairs such that each pair has a + *resource ID and its requested access mode. + * @return True if the transaction is compatible and false if it is not. + */ + virtual bool isTranasctionCompatible(const transaction_id tid, + const std::vector> &resource_requests) { +return false; + } + + /** + * @brief Check if the given transaction can acquire a Catalog lock on the + *given relation. + * @param tid The ID of the given transaction. + * @param relation The given relation. + * @param access_mode The access_mode requested to lock the catalog. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireCatalogLock(const transaction_id tid, + const CatalogRelation &relation, + const AccessMode access_mode) { +return false; + } + + /** + * @brief Check if the given transaction can acquire locks on all the blocks + *of the given relation. + * @param tid The ID of the given transaction. + * @param rid The ID of the given relation. + * @param blocks The list of the blocks of the given relation. + * @param access_mode The access_mode requested to lock the blocks. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireBlockLocks(const transaction_id tid, +const relation_id rid, +const std::vector &blocks, +const AccessMode access_mode) { +return false; + } --- End diff -- Optionally, mark as `pure virtual` methods. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019788 --- Diff: transaction/AdmissionControl.hpp --- @@ -0,0 +1,83 @@ +/** + * 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_TRANSACTION_ADMISSION_CONTROL_HPP_ +#define QUICKSTEP_TRANSACTION_ADMISSION_CONTROL_HPP_ + +#include "utility/Macros.hpp" +#include "transaction/TransactionTable.hpp" + +namespace quickstep { +namespace transaction { + +class AdmissionControl { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + AdmissionControl(TransactionTable *transaction_table) {} --- End diff -- Add `explicit`. I was wondering which object will own `transaction_table`. And does this class need to store the pointer? Also, should this class contain the `CompatibilityChecker` object? ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019935 --- Diff: transaction/CMakeLists.txt --- @@ -50,10 +53,16 @@ add_library(quickstep_transaction_StronglyConnectedComponents add_library(quickstep_transaction_Transaction ../empty_src.cpp Transaction.hpp) +add_library(quickstep_transaction_CompatibilityChecker --- End diff -- Alphabet order. Ditto below. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153020184 --- Diff: transaction/CompatibilityChecker.hpp --- @@ -0,0 +1,106 @@ +/** + * 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_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ +#define QUICKSTEP_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ + +#include +#include + +#include "transaction/TransactionTable.hpp" +#include "utility/Macros.hpp" + +namespace quickstep { + +class CatalogRelation; + +namespace transaction { + +/** + * @brief A class that checks the compatibility of a transaction with the + *other running transactions. + */ +class CompatibilityChecker { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + CompatibilityChecker(TransactionTable *transaction_table) {} + + virtual ~CompatibilityChecker() {} + + /** + * @brief Check if the given transaction is compatible with other + *running transactions. + * @note The transaction table has the list of all the running or pending + * transactions. Lookup the table and check if the given transaction + * is compatible, based on the CC protocol specifications. + * @note Accesses to the transaction_table may need protection. + + * @param tid The ID of the given transaction + * @param resource_requests A vector of pairs such that each pair has a + *resource ID and its requested access mode. + * @return True if the transaction is compatible and false if it is not. + */ + virtual bool isTranasctionCompatible(const transaction_id tid, + const std::vector> &resource_requests) { +return false; + } + + /** + * @brief Check if the given transaction can acquire a Catalog lock on the + *given relation. + * @param tid The ID of the given transaction. + * @param relation The given relation. + * @param access_mode The access_mode requested to lock the catalog. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireCatalogLock(const transaction_id tid, + const CatalogRelation &relation, + const AccessMode access_mode) { +return false; + } + + /** + * @brief Check if the given transaction can acquire locks on all the blocks + *of the given relation. + * @param tid The ID of the given transaction. + * @param rid The ID of the given relation. + * @param blocks The list of the blocks of the given relation. + * @param access_mode The access_mode requested to lock the blocks. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireBlockLocks(const transaction_id tid, +const relation_id rid, +const std::vector &blocks, +const AccessMode access_mode) { +return false; + } + + private: + DISALLOW_COPY_AND_ASSIGN(CompatibilityChecker); + +}; + +} // namespace transaction +} // namespace quickstep + +#endif //QUICKSTEP_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ --- End diff -- Add a whitespace after `//`, and add `EOL` in the end. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019545 --- Diff: transaction/AdmissionControl.hpp --- @@ -0,0 +1,83 @@ +/** + * 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_TRANSACTION_ADMISSION_CONTROL_HPP_ +#define QUICKSTEP_TRANSACTION_ADMISSION_CONTROL_HPP_ + +#include "utility/Macros.hpp" +#include "transaction/TransactionTable.hpp" --- End diff -- Alphabet order. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153020141 --- Diff: transaction/CompatibilityChecker.hpp --- @@ -0,0 +1,106 @@ +/** + * 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_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ +#define QUICKSTEP_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ + +#include +#include + +#include "transaction/TransactionTable.hpp" +#include "utility/Macros.hpp" + +namespace quickstep { + +class CatalogRelation; + +namespace transaction { + +/** + * @brief A class that checks the compatibility of a transaction with the + *other running transactions. + */ +class CompatibilityChecker { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + CompatibilityChecker(TransactionTable *transaction_table) {} + + virtual ~CompatibilityChecker() {} + + /** + * @brief Check if the given transaction is compatible with other + *running transactions. + * @note The transaction table has the list of all the running or pending + * transactions. Lookup the table and check if the given transaction + * is compatible, based on the CC protocol specifications. + * @note Accesses to the transaction_table may need protection. + + * @param tid The ID of the given transaction + * @param resource_requests A vector of pairs such that each pair has a + *resource ID and its requested access mode. + * @return True if the transaction is compatible and false if it is not. + */ + virtual bool isTranasctionCompatible(const transaction_id tid, + const std::vector> &resource_requests) { +return false; + } + + /** + * @brief Check if the given transaction can acquire a Catalog lock on the + *given relation. + * @param tid The ID of the given transaction. + * @param relation The given relation. + * @param access_mode The access_mode requested to lock the catalog. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireCatalogLock(const transaction_id tid, + const CatalogRelation &relation, + const AccessMode access_mode) { +return false; + } + + /** + * @brief Check if the given transaction can acquire locks on all the blocks + *of the given relation. + * @param tid The ID of the given transaction. + * @param rid The ID of the given relation. + * @param blocks The list of the blocks of the given relation. + * @param access_mode The access_mode requested to lock the blocks. + * @return True if the lock can be acquired, false otherwise. + */ + virtual bool canAcquireBlockLocks(const transaction_id tid, +const relation_id rid, +const std::vector &blocks, +const AccessMode access_mode) { +return false; + } + + private: + DISALLOW_COPY_AND_ASSIGN(CompatibilityChecker); + --- End diff -- Remove this empty line. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019502 --- Diff: query_optimizer/QueryHandle.hpp --- @@ -178,6 +178,14 @@ class QueryHandle { query_result_relation_ = relation; } + /** + * @brief Return all the base relations referenced in this query. + **/ + std::vector getAllReferencedBaseRelations() { --- End diff -- This should mark as a `const` method, and I think we need to add a data member called `referenced_base_relations_`. ---
[GitHub] incubator-quickstep pull request #325: DO NOT MERGE: Concurrent queries tran...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/325#discussion_r153019995 --- Diff: transaction/CompatibilityChecker.hpp --- @@ -0,0 +1,106 @@ +/** + * 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_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ +#define QUICKSTEP_TRANSACTION_TRANSACTION_COMPATIBILITY_CHECKER_HPP_ + +#include +#include + +#include "transaction/TransactionTable.hpp" +#include "utility/Macros.hpp" + +namespace quickstep { + +class CatalogRelation; + +namespace transaction { + +/** + * @brief A class that checks the compatibility of a transaction with the + *other running transactions. + */ +class CompatibilityChecker { + public: + /** + * @brief Constructor + * @param transaction_table A lookup table that stores information about + *all the running and waiting transactions. + */ + CompatibilityChecker(TransactionTable *transaction_table) {} --- End diff -- Add `explicit`. ---
[GitHub] incubator-quickstep issue #300: QUICKSTEP-106: Hash-Join-Fuse: Feature added...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/300 Hi @jianqiao Could you explain more regarding the suspect reason of the slowdown in the general case (i.e., w/o the threshold)? Thanks! ---
[GitHub] incubator-quickstep issue #323: Temporary Build Support for OS X 10.13
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/323 @robertclaus What's going on? ---
[GitHub] incubator-quickstep pull request #323: Temporary Build Support for OS X 10.1...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/323#discussion_r150140641 --- Diff: CMakeLists.txt --- @@ -302,11 +302,11 @@ else() endif() endif() - # OSX 10.12 has deprecated certain system-level APIs which causes protobuf & glog + # OSX 10.12+ has deprecated certain system-level APIs which causes protobuf & glog # builds to fail. As a short-term workaround for now, we turn off deprecated # warnings so that they do not cause build failures anymore. # TODO: Remove this workaround by fixing the protobuf_cmake and glog_cmake. - if (${CMAKE_SYSTEM} MATCHES "Darwin-16.[0-9]*.[0-9]*") + if (${CMAKE_SYSTEM} MATCHES "Darwin-1[6-7]*.[0-9]*.[0-9]*") --- End diff -- Just curious if you have tried `Darwin-1[67].[0-9]*.[0-9]*`? Thanks! ---
[GitHub] incubator-quickstep issue #323: Temporary Build Support for OS X 10.13
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/323 Hi Yuanchen, Thank you for submitting your PR, and we will test your PR soon. ---
[GitHub] incubator-quickstep pull request #322: Generalized Hash Join - DO NOT MERGE
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/322#discussion_r149791914 --- Diff: query_optimizer/ExecutionGenerator.cpp --- @@ -1102,6 +1121,320 @@ void ExecutionGenerator::convertHashJoin(const P::HashJoinPtr &physical_plan) { } } +void ExecutionGenerator::convertGeneralizedHashJoin(const P::GeneralizedHashJoinPtr &physical_plan) { + // HashJoin is converted to three operators: + // BuildHash, HashJoin, DestroyHash. The second is the primary operator. + + P::PhysicalPtr probe_physical = physical_plan->left(); + P::PhysicalPtr build_physical = physical_plan->right(); + P::PhysicalPtr second_build_physical = physical_plan->middle(); + + std::vector probe_attribute_ids; + std::vector build_attribute_ids; + std::vector second_probe_attribute_ids; + std::vector second_build_attribute_ids; + + std::size_t build_cardinality = + cost_model_for_hash_join_->estimateCardinality(build_physical); + + std::size_t second_build_cardinality = + cost_model_for_hash_join_->estimateCardinality(second_build_physical); + + bool any_probe_attributes_nullable = false; + bool any_build_attributes_nullable = false; + bool any_second_probe_attributes_nullable = false; + bool any_second_build_attributes_nullable = false; + + const std::vector &left_join_attributes = + physical_plan->left_join_attributes(); + for (const E::AttributeReferencePtr &left_join_attribute : left_join_attributes) { --- End diff -- We could create functions in the anonymous namespace to set these values. ---
[GitHub] incubator-quickstep issue #320: Support Multiple Tuple Inserts
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/320 IMHO, this PR has a major design issue where for multi-tuple insertions, we create a bunch of `InsertOperator` and `SaveBlocksOperator`, each for tuple. ---
[GitHub] incubator-quickstep pull request #319: Fixed the bug when partition w/ prune...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/319 Fixed the bug when partition w/ pruned columns. This PR fixed the bug that when some column gets pruned, the partition is incorrect due to the wrong logical catalog attribute id from the output relation. We should use the old logical catalog attribute id from *the input relation*. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep fix-pruned-col-w-partitions Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/319.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 #319 commit 8b0b05ca259ed552917a8a9bcc7fed7d28d541ea Author: Zuyu Zhang Date: 2017-10-24T21:17:50Z Fixed the bug when partition w/ pruned columns. ---
[GitHub] incubator-quickstep pull request #318: Optimizing Hash repartition on a raw ...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/318 Optimizing Hash repartition on a raw pointer, instead of using TypedValue. This PR optimizes hash repartition over a raw pointer, instead of `TypedValue`. Experimental results show that repartition a `lineorder` from `SSB-100` on would reduce to `750 ms` from `1000 ms`. Note that this PR allows supports the single partition attribute. For multi-attribute repartition, there are two basic approaches. 1. like the single partition optimization, combine per-column hash, and then set the bit vector for the result partition. 1. per-tuple evaluation, w/o any extra buffer used in the above approach. The first approach on the single partition attribute actually takes `1200 ms`, while the latter takes `850 ms`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep hash-p-void-single Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/318.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 #318 ---
[GitHub] incubator-quickstep pull request #317: Fixed the include path for farmhash.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/317 Fixed the include path for farmhash. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep farmhash-include-path Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/317.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 #317 commit 79bfcf9ed294477a24823b00bd814df0de54ee5e Author: Zuyu Zhang Date: 2017-10-13T21:07:51Z Fixed the include path for farmhash. ---
[GitHub] incubator-quickstep issue #309: Added ProbabilityStore class
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/309 LGTM. Merging. ---
[GitHub] incubator-quickstep pull request #308: Removed unused argument always_mark_f...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/308#discussion_r143825045 --- Diff: storage/InsertDestinationInterface.hpp --- @@ -104,7 +104,7 @@ class InsertDestinationInterface { *insertion from ValueAccessor even when partially full. **/ virtual void bulkInsertTuples(ValueAccessor *accessor, -bool always_mark_full = false) = 0; +const bool always_mark_full = false) = 0; --- End diff -- It is required by the first pass of the multi-pass sort. ---
[GitHub] incubator-quickstep pull request #314: Added Vector Aggregation support in t...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/314 Added Vector Aggregation support in the distributed version. Assigned to @jianqiao. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep dist-vector-aggr Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/314.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 #314 commit 705ab574e606a859a1a1bfe8a2dd9a62f796b1bd Author: Zuyu Zhang Date: 2017-08-04T22:03:34Z Added Vector Aggregation support in the distributed version. ---
[GitHub] incubator-quickstep pull request #313: Relax the sort requirement in columns...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/313 Relax the sort requirement in columnstore. This small PR allows to create a relation w/ column store, while not specifying a sort attribute. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep no-sort-in-cs Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/313.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 #313 commit 2e0496c22a804da067a91caf8bc291b8c66a9c7b Author: Zuyu Zhang Date: 2017-10-09T16:23:08Z Relax the sort requirement in columnstore. ---
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143527465 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143522173 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); --- End diff -- Does it follow the rule that the left side is the checked variable, and the right is the expected one? ---
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143522609 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143525719 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143522055 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { --- End diff -- Remove `const` in the return type. ---
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143528439 --- Diff: query_execution/tests/ProbabilityStore_unittest.cpp --- @@ -0,0 +1,106 @@ +/** + * 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. + **/ + +#include +#include + +#include "gtest/gtest.h" + +#include "query_execution/ProbabilityStore.hpp" --- End diff -- Per header order, move it to the top. ---
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143527518 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143527323 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/309#discussion_r143524550 --- Diff: query_execution/ProbabilityStore.hpp --- @@ -0,0 +1,274 @@ +/** + * 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_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_PROBABILITY_STORE_HPP_ + +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** + * @brief A class that stores the probabilities of objects. We use an integer field + *called "key" to identify each object. + *A probability is expressed as a fraction. All the objects share a common denominator. + **/ +class ProbabilityStore { + public: + /** + * @brief Constructor. + **/ + ProbabilityStore() + : common_denominator_(1.0) {} + + /** + * @brief Get the number of objects in the store. + **/ + inline const std::size_t getNumObjects() const { +DCHECK_EQ(individual_probabilities_.size(), cumulative_probabilities_.size()); +return individual_probabilities_.size(); + } + + /** + * @brief Get the common denominator. + */ + inline const std::size_t getDenominator() const { +return common_denominator_; + } + + /** + * @brief Check if an object with a given key is present. + * @param key The key of the given object. + * @return True if the object is present, false otherwise. + */ + inline bool hasObject(const std::size_t key) const { +return (individual_probabilities_.find(key) != individual_probabilities_.end()); + } + + /** + * @brief Add individual (not cumulative) probability for a given object with + *updated denominator. + * + * @note This function leaves the cumulative probabilities in a consistent + * state. An alternative lazy implementation should be written if cost + * of calculating cumulative probabilities is high. + * + * @param key The key of the given object. + * @param numerator The numerator for the given object. + * @param new_denominator The updated denominator for the store. + **/ + void addOrUpdateObjectNewDenominator(const std::size_t key, + const float numerator, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +DCHECK_LE(numerator, new_denominator); +common_denominator_ = new_denominator; +addOrUpdateObjectHelper(key, numerator); + } + + /** + * @brief Add or update multiple objects with a new denominator. + * @param keys The keys to be added/updated. + * @param numerators The numerators to be added/updated. + * @param new_denominator The new denominator. + */ + void addOrUpdateObjectsNewDenominator( + const std::vector &keys, + const std::vector &numerators, + const float new_denominator) { +CHECK_GT(new_denominator, 0u); +common_denominator_ = new_denominator; +addOrUpdateObjectsHelper(keys, numerators); + } + + /** + * @brief Remove an object from the store. + * + * @note This function decrements the denominator with the value of the numerator being removed. + * + * @param key The key of the object to be removed. + **/ + void removeObject(const std::size_t key) { +auto individual_it = individual_probabilities_.find(key); +DCHECK(individual_it != individual_probabilities_.end()); +const float new_denominator = common_denominator_ - individual_it->second; +individual_probabilities_.erase(individual_it); +common_denominat
[GitHub] incubator-quickstep pull request #312: Fixed a flaky case in Catalog test.
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/312 Fixed a flaky case in Catalog test. This small PR fixes a flaky test case of adding an index with a different name, but the same description. However, the adding description is empty due to previous move semantics. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep fix-catalog-test Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/312.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 #312 commit 45744d4d30f61183602aa64aeda4ae0cc02556f9 Author: Zuyu Zhang Date: 2017-10-08T19:20:41Z Fixed a flaky case in Catalog test. ---
[GitHub] incubator-quickstep pull request #311: Fixed the distributed version
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/311 Fixed the distributed version due to query execution engine simplification #281. Note that Travis CI does not test the distributed version. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep dist-qe-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/311.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 #311 commit e496cb58e10d32de9dc83d69ece84df3f5b62747 Author: Zuyu Zhang Date: 2017-10-07T03:33:02Z Fixed the distributed version due to query execution engine simplification. ---
[GitHub] incubator-quickstep pull request #310: Removed the virtual function call in ...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/310 Removed the virtual function call in InvokeOnAnyValueAccessor. A minor fix. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep va-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/310.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 #310 commit 8a91b12054f534a5b94321bb087168a4372883f8 Author: Zuyu Zhang Date: 2017-10-06T19:34:21Z Removed the virtual function call in InvokeOnAnyValueAccessor. ---
[GitHub] incubator-quickstep pull request #308: Removed unused argument always_mark_f...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/308 Removed unused argument always_mark_full. This PR cleans up the unused argument `always_mark_full`, and avoids branching overhead. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep always-mark-full-arg Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/308.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 #308 commit f4c27de2e19f56a0410c9c216a7b96604358b50b Author: Zuyu Zhang Date: 2017-10-05T22:18:05Z Removed unused argument always_mark_full. ---
[GitHub] incubator-quickstep pull request #307: Moved InsertDestination::getTouchedBl...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/307 Moved InsertDestination::getTouchedBlocks as a private method. The method is used only for testing. Assigned to @hbdeshmukh. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep get-touched-blocks Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/307.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 #307 ---
[GitHub] incubator-quickstep pull request #306: Fixed the root path check in the cycl...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/306 Fixed the root path check in the cyclic_dependency.py. A minor fix. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep fix-script Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/306.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 #306 commit 8adb03196abcb0957494ee6201169cd75622c20c Author: Zuyu Zhang Date: 2017-10-03T01:47:44Z Fixed the root path check in the cyclic_dependency.py. ---
[GitHub] incubator-quickstep pull request #305: Optimized the mod operation in HashPa...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/305 Optimized the mod operation in HashPartition. This small PR optimized out the mod operation `%` when possible. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep optimize-mod Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/305.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 #305 commit 35e56ea5e2cf002d5a405d74cfabd255ae7dee7c Author: Zuyu Zhang Date: 2017-09-29T20:37:14Z Optimized the mod operation in HashPartition. ---
[GitHub] incubator-quickstep pull request #304: Added a new set API for TupleIdSequen...
GitHub user zuyu opened a pull request: https://github.com/apache/incubator-quickstep/pull/304 Added a new set API for TupleIdSequence. This PR added a new `set` API for `TupleIdSequence`, and has some performance improvement. Assigned to @jianqiao. ![chart-3](https://user-images.githubusercontent.com/2245572/31027397-b7e2c836-a50f-11e7-9910-bdc8f1f1e825.png) You can merge this pull request into a Git repository by running: $ git pull https://github.com/zuyu/incubator-quickstep simplified-set-tuple-id Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/304.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 #304 commit 82341835aaeb05525dd7822935bd0729726d54c0 Author: Zuyu Zhang Date: 2017-09-29T00:28:30Z Added a new set API for TupleIdSequence. ---
[GitHub] incubator-quickstep issue #302: Created a class to track execution statistic...
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/302 LGTM! Merging. ---
[GitHub] incubator-quickstep issue #301: Bug fix in LockManager loop
Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/301 LGTM! ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141769721 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { --- End diff -- We may typedef `std::pair`. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141769962 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { +auto result = getCurrentStatsForQuery(); +if (result.second != 0) { + return result.first/static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Get the current stats for the given operator. + * @param operator_id The ID of the operator. + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the operator. + */ + std::pair getCurrentStatsForOperator(std::size_t operator_id) const { +if (hasOperator(operator_id)) { + DCHECK(active_operators_.at(operator_id).get() != nullptr); --- End diff -- Optionally, remove `.get()`, as `unique_ptr` has `operator!=`. `DCHECK(active_operators_.at(operator_id) != nullptr);`. Alternatively, `DCHECK(active_operators_.at(operator_id));` may work as well. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141770622 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { +auto result = getCurrentStatsForQuery(); +if (result.second != 0) { + return result.first/static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Get the current stats for the given operator. + * @param operator_id The ID of the operator. + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the operator. + */ + std::pair getCurrentStatsForOperator(std::size_t operator_id) const { +if (hasOperator(operator_id)) { + DCHECK(active_operators_.at(operator_id).get() != nullptr); + return active_operators_.at(operator_id)->getStats(); +} +return std::make_pair(0, 0); + } + + float getAverageWorkOrderTimeForOperator(std::size_t operator_id) const { +auto result = getCurrentStatsForOperator(operator_id); +if (result.second != 0) { + return result.first / static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Add a new entry to stats. + * + * @param value The value to be added. + * @param operator_index The operator index which the value belongs to. + **/ + void addEntry(std::size_t value, std::size_t operator_index) { +if (!hasOperator(operator_index)) { + // This is the first entry for the given operator. + // Create the OperatorStats object for this operator. + ac
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141770165 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { +auto result = getCurrentStatsForQuery(); +if (result.second != 0) { + return result.first/static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Get the current stats for the given operator. + * @param operator_id The ID of the operator. + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the operator. + */ + std::pair getCurrentStatsForOperator(std::size_t operator_id) const { +if (hasOperator(operator_id)) { + DCHECK(active_operators_.at(operator_id).get() != nullptr); + return active_operators_.at(operator_id)->getStats(); +} +return std::make_pair(0, 0); + } + + float getAverageWorkOrderTimeForOperator(std::size_t operator_id) const { +auto result = getCurrentStatsForOperator(operator_id); +if (result.second != 0) { + return result.first / static_cast(result.second); --- End diff -- Ditto for `double`. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141768980 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); --- End diff -- Remove an unnecessary pair of `()`. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141769628 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { +auto result = getCurrentStatsForQuery(); +if (result.second != 0) { + return result.first/static_cast(result.second); --- End diff -- Since `result.second` is an `uint64_t`, we have to cast to `double`. Also, please add ` ` between `/` for readability. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141771086 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { --- End diff -- I suggest to use `double`, to be consistent with `uint64_t`. ---
[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...
Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/302#discussion_r141770914 --- Diff: query_execution/ExecutionStats.hpp --- @@ -0,0 +1,210 @@ +/** + * 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_QUERY_EXECUTION_EXECUTION_STATS_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_EXECUTION_STATS_HPP_ + +#include +#include +#include +#include +#include +#include +#include + +#include "utility/Macros.hpp" + +#include "glog/logging.h" + +namespace quickstep { + +/** \addtogroup QueryExecution + * @{ + */ + +/** + * @brief Record the execution stats of a query. + **/ +class ExecutionStats { + public: + /** + * @brief Constructor + * + * @param max_entries The maximum number of entries we remember for each + *operator. + **/ + explicit ExecutionStats(const std::size_t max_entries) + : max_entries_(max_entries) {} + + /** + * @brief Get the number of active operators in stats. + **/ + const std::size_t getNumActiveOperators() const { +return active_operators_.size(); + } + + /** + * @brief Check if there are stats present for at least one active operator. + **/ + inline bool hasStats() const { +for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { + if (it->second->hasStatsForOperator()) { +return true; + } +} +return false; + } + + /** + * @brief Get the current stats for the query. + * + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the whole query. + **/ + std::pair getCurrentStatsForQuery() const { +std::uint64_t total_time = 0; +std::uint64_t num_workorders = 0; +if (!active_operators_.empty()) { + for (auto it = active_operators_.begin(); it != active_operators_.end(); ++it) { +auto operator_stats = getCurrentStatsForOperator((it->first)); +total_time += operator_stats.first; +num_workorders += operator_stats.second; + } +} +return std::make_pair(total_time, num_workorders); + } + + /** + * @brief Get the average work order time for the query. + */ + float getAverageWorkOrderTimeForQuery() const { +auto result = getCurrentStatsForQuery(); +if (result.second != 0) { + return result.first/static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Get the current stats for the given operator. + * @param operator_id The ID of the operator. + * @return A pair - 1st element is total time, 2nd element is total number of + * WorkOrders for the operator. + */ + std::pair getCurrentStatsForOperator(std::size_t operator_id) const { +if (hasOperator(operator_id)) { + DCHECK(active_operators_.at(operator_id).get() != nullptr); + return active_operators_.at(operator_id)->getStats(); +} +return std::make_pair(0, 0); + } + + float getAverageWorkOrderTimeForOperator(std::size_t operator_id) const { +auto result = getCurrentStatsForOperator(operator_id); +if (result.second != 0) { + return result.first / static_cast(result.second); +} +return 0.0; + } + + /** + * @brief Add a new entry to stats. + * + * @param value The value to be added. + * @param operator_index The operator index which the value belongs to. + **/ + void addEntry(std::size_t value, std::size_t operator_index) { +if (!hasOperator(operator_index)) { + // This is the first entry for the given operator. + // Create the OperatorStats object for this operator. + ac