Re: [DISCUSS] Retire Quickstep podling from Apache Incubator

[Zuyu Zhang]

+1 to retire.

Cheers,
Zuyu

[GitHub] incubator-quickstep issue #362: Add virtual destructors where needed to remo...

[zuyu]

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 ...

[zuyu]

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 issue #362: Add virtual destructors where needed to remo...

[zuyu]

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...

[zuyu]

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 ...

[zuyu]

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

[zuyu]

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

[zuyu]

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...

[zuyu]

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 <zuyu@...>
Date:   2018-05-21T21:39:42Z

Fixed the command execution bug in the distributed version.




---

[GitHub] incubator-quickstep pull request #356: DO NOT MERGE: Added the grpc in Travi...

[zuyu]

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 <zuyu@...>
Date:   2018-05-21T18:42:15Z

Added the grpc in Travis CI.




---

[GitHub] incubator-quickstep issue #355: QUICKSTEP-127 Data provider thread

[zuyu]

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

[zuyu]

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...

[zuyu]

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 <zuyu@...>
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.

[zuyu]

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 <zuyu@...>
Date:   2018-05-07T21:08:35Z

Minor bug fixes and refactors.




---

[GitHub] incubator-quickstep pull request #351: Use Exactness info in Catalog stats.

[zuyu]

Github user zuyu closed the pull request at:

https://github.com/apache/incubator-quickstep/pull/351


---

[GitHub] incubator-quickstep pull request #350: Fixed the bug regarding EliminateEmpt...

[zuyu]

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 <zuyu@...>
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...

[zuyu]

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...

[zuyu]

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 <zuyu@...>
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.

[zuyu]

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.

[zuyu]

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 <zuyu@...>
Date:   2018-05-02T21:06:59Z

Refactored ScalarCaseExpression.




---

[GitHub] incubator-quickstep pull request #347: DO NOT MERGE: QUICKSTEP-121: Added th...

[zuyu]

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 
`<build-tid, probe-tid>`.

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 <zuyu@...>
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...

[zuyu]

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...

[zuyu]

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.

[zuyu]

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 <zuyu@...>
Date:   2018-04-27T02:39:18Z

Code style fix.




---

[GitHub] incubator-quickstep pull request #344: Quickstep-123: Fixed the missing 'has...

[zuyu]

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 <zuyu@...>
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...

[zuyu]

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 <zuyu@...>
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.

[zuyu]

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.

[zuyu]

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 <zuyu@...>
Date:   2018-04-20T20:46:50Z

Used MergeFrom instead of CopyFrom.




---

[GitHub] incubator-quickstep issue #338: Fixed the gRPC Problem for Data Exchange

[zuyu]

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...

[zuyu]

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...

[zuyu]

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...

[zuyu]

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 <zuyu@...>
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...

[zuyu]

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 <zuyu@...>
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...

[zuyu]

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

[zuyu]

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...

[zuyu]

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 ...

[zuyu]

Github user zuyu commented on the issue:

https://github.com/apache/incubator-quickstep/pull/332
  
LGTM!


---

[GitHub] incubator-quickstep issue #333: Fix SeparateChainingHashTable::resize()

[zuyu]

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...

[zuyu]

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...

[zuyu]

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...

[zuyu]

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

[zuyu]

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

[zuyu]

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...

[zuyu]

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...

[zuyu]

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<std::pair<ResourceId, 
AccessMode>> _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...

[zuyu]

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...

[zuyu]

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<std::pair<ResourceId, AccessMode>> _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 ,
+ 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 ,
+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...

[zuyu]

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<std::pair<ResourceId, AccessMode>> _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 ,
+ 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 ,
+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...

[zuyu]

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...

[zuyu]

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...

[zuyu]

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...

[zuyu]

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 issue #300: QUICKSTEP-106: Hash-Join-Fuse: Feature added...

[zuyu]

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

[zuyu]

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...

[zuyu]

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

[zuyu]

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

[zuyu]

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 _plan) {
   }
 }
 
+void ExecutionGenerator::convertGeneralizedHashJoin(const 
P::GeneralizedHashJoinPtr _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 _join_attributes =
+  physical_plan->left_join_attributes();
+  for (const E::AttributeReferencePtr _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

[zuyu]

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...

[zuyu]

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 <z...@cs.wisc.edu>
Date:   2017-10-24T21:17:50Z

Fixed the bug when partition w/ pruned columns.




---

[GitHub] incubator-quickstep pull request #317: Fixed the include path for farmhash.

[zuyu]

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 <z...@cs.wisc.edu>
Date:   2017-10-13T21:07:51Z

Fixed the include path for farmhash.




---

[GitHub] incubator-quickstep issue #309: Added ProbabilityStore class

[zuyu]

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...

[zuyu]

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...

[zuyu]

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 <z...@cs.wisc.edu>
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...

[zuyu]

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 <z...@cs.wisc.edu>
Date:   2017-10-09T16:23:08Z

Relax the sort requirement in columnstore.




---

[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class

[zuyu]

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 ,
+  const std::vector ,
+  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_denominator_ = new_denominator;
+updateCumulativeProbab

[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class

[zuyu]

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

[zuyu]

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 ,
+  const std::vector ,
+  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_denominator_ = new_denominator;
+updateCumulativeProbab

[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class

[zuyu]

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 ,
+  const std::vector ,
+  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_denominator_ = new_denominator;
+updateCumulativeProbab

[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class

[zuyu]

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

[zuyu]

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

[zuyu]

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 ,
+  const std::vector ,
+  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_denominator_ = new_denominator;
+updateCumulativeProbab

[GitHub] incubator-quickstep pull request #309: Add ProbabilityStore class

[zuyu]

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 ,
+  const std::vector ,
+  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_denominator_ = new_denominator;
+updateCumulativeProbab

[GitHub] incubator-quickstep pull request #312: Fixed a flaky case in Catalog test.

[zuyu]

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 <z...@cs.wisc.edu>
Date:   2017-10-08T19:20:41Z

Fixed a flaky case in Catalog test.




---

[GitHub] incubator-quickstep pull request #311: Fixed the distributed version

[zuyu]

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 <z...@cs.wisc.edu>
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 ...

[zuyu]

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 <z...@cs.wisc.edu>
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...

[zuyu]

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 <z...@cs.wisc.edu>
Date:   2017-10-05T22:18:05Z

Removed unused argument always_mark_full.




---

[GitHub] incubator-quickstep pull request #307: Moved InsertDestination::getTouchedBl...

[zuyu]

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...

[zuyu]

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 <z...@cs.wisc.edu>
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...

[zuyu]

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 <z...@cs.wisc.edu>
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...

[zuyu]

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 <z...@cs.wisc.edu>
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...

[zuyu]

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

[zuyu]

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...

[zuyu]

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<std::uint64_t, std::uint64_t> getCurrentStatsForQuery() const {
--- End diff --

We may typedef `std::pair<std::uint64_t, std::uint64_t>`.


---

[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...

[zuyu]

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<std::uint64_t, std::uint64_t> 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<std::uint64_t, std::uint64_t> 
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...

[zuyu]

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<std::uint64_t, std::uint64_t> 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<std::uint64_t, std::uint64_t> 
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.

[GitHub] incubator-quickstep pull request #302: Created a class to track execution st...

[zuyu]

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<std::uint64_t, std::uint64_t> 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<std::uint64_t, std::uint64_t> 
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...

[zuyu]

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<std::uint64_t, std::uint64_t> 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...

[zuyu]

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<std::uint64_t, std::uint64_t> 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...

[zuyu]

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<std::uint64_t, std::uint64_t> 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...

[zuyu]

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<std::uint64_t, std::uint64_t> 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<std::uint64_t, std::uint64_t> 
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.

[GitHub] incubator-quickstep pull request #301: Bug fix in LockManager loop

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/301#discussion_r141768057
  
--- Diff: transaction/LockManager.cpp ---
@@ -79,22 +79,23 @@ void LockManager::run() {
 const LockRequest request = incoming_requests_.popOne();
 if (request.getRequestType() == RequestType::kReleaseLocks) {
   CHECK(releaseAllLocks(request.getTransactionId()))
-  << "Unexpected condition occured.";
-
+  << "Unexpected condition occured.";
--- End diff --

I think the original style is correct.


---

[GitHub] incubator-quickstep pull request #301: Bug fix in LockManager loop

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/301#discussion_r141768111
  
--- Diff: transaction/LockManager.cpp ---
@@ -79,22 +79,23 @@ void LockManager::run() {
 const LockRequest request = incoming_requests_.popOne();
 if (request.getRequestType() == RequestType::kReleaseLocks) {
   CHECK(releaseAllLocks(request.getTransactionId()))
-  << "Unexpected condition occured.";
-
+  << "Unexpected condition occured.";
 } else if (acquireLock(request.getTransactionId(),
-   request.getResourceId(),
-   request.getAccessMode())) {
+request.getResourceId(),
+request.getAccessMode())) {
--- End diff --

Please align with the first argument.


---

[GitHub] incubator-quickstep issue #300: QUICKSTEP-106: Hash-Join-Fuse: Feature added...

[zuyu]

Github user zuyu commented on the issue:

https://github.com/apache/incubator-quickstep/pull/300
  
We need to fix the performance bugs before merging.

|  **TPC-H** | **master** | **this PR** |
|  -- | --: | --: |
|  01 | 5,421 | 5,039 |
|  02 | 402 | 3,968 |
|  03 | 1,229 | 3,506 |
|  04 | 2,472 | 1,242 |
|  05 | 2,807 | 3,731 |
|  06 | 399 | 394 |
|  07 | 1,754 | 3,432 |
|  08 | 906 | 9,961 |
|  09 | 6,766 | TO |


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140575177
  
--- Diff: query_optimizer/rules/FuseHashSelect.hpp ---
@@ -0,0 +1,67 @@
+/**
+ * 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_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_
+#define QUICKSTEP_QUERY_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_
+
+#include 
+#include 
+#include 
+
+
--- End diff --

Remove this duplicated empty line, and unused headers, if any.


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140569985
  
--- Diff: query_optimizer/ExecutionGenerator.cpp ---
@@ -939,6 +939,15 @@ void ExecutionGenerator::convertHashJoin(const 
P::HashJoinPtr _plan) {
 
query_context_proto_->add_predicates()->CopyFrom(residual_predicate->getProto());
   }
 
+  // Convert the build predicate proto.
+  QueryContext::predicate_id build_predicate_index = 
QueryContext::kInvalidPredicateId;
+  if (physical_plan->build_predicate()) {
+build_predicate_index = query_context_proto_->predicates_size();
+
+unique_ptr 
build_predicate(convertPredicate(physical_plan->build_predicate()));
+
query_context_proto_->add_predicates()->CopyFrom(build_predicate->getProto());
--- End diff --

Change to `MergeFrom` to avoid an unnecessary reset function call.


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140575077
  
--- Diff: query_optimizer/rules/FuseHashSelect.cpp ---
@@ -0,0 +1,91 @@
+/**
+ * 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 "query_optimizer/rules/FuseHashSelect.hpp"
+
+#include 
+#include 
+#include 
+#include 
+
+#include "query_optimizer/expressions/AttributeReference.hpp"
+#include "query_optimizer/expressions/ExpressionUtil.hpp"
+#include "query_optimizer/expressions/PatternMatcher.hpp"
+#include "query_optimizer/expressions/Predicate.hpp"
+#include "query_optimizer/physical/HashJoin.hpp"
+#include "query_optimizer/physical/PatternMatcher.hpp"
+#include "query_optimizer/physical/Physical.hpp"
+#include "query_optimizer/physical/PhysicalType.hpp"
+#include "query_optimizer/physical/Selection.hpp"
+
+#include "glog/logging.h"
--- End diff --

Do we actually use sth from all these headers above? If no, please remove 
the unused headers.


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140570548
  
--- Diff: query_optimizer/PhysicalGenerator.cpp ---
@@ -175,6 +176,8 @@ P::PhysicalPtr PhysicalGenerator::optimizePlan() {
 rules.push_back(std::make_unique(optimizer_context_));
   }
 
+  rules.emplace_back(new FuseHashSelect());
--- End diff --

You may want to add a `gflags` with a default `true` value to switch 
between using this rule or not, w/o rebuilding the binary.


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140575916
  
--- Diff: query_optimizer/strategy/Join.cpp ---
@@ -371,6 +373,7 @@ void Join::addHashJoin(const logical::ProjectPtr 
_project,
   left_join_attributes,
   right_join_attributes,
   residual_predicate,
+  build_predicate,
--- End diff --

Optionally, I think `E::PredicatePtr()  /* build_predicate */,` is better.


---

[GitHub] incubator-quickstep pull request #300: QUICKSTEP-106: Hash-Join-Fuse: Featur...

[zuyu]

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

https://github.com/apache/incubator-quickstep/pull/300#discussion_r140575606
  
--- Diff: query_optimizer/rules/FuseHashSelect.hpp ---
@@ -0,0 +1,67 @@
+/**
+ * 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_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_
+#define QUICKSTEP_QUERY_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_
+
+#include 
+#include 
+#include 
+
+
+#include "query_optimizer/physical/HashJoin.hpp"
+#include "query_optimizer/physical/Physical.hpp"
+#include "query_optimizer/rules/TopDownRule.hpp"
+#include "utility/Macros.hpp"
+
+namespace quickstep {
+namespace optimizer {
+
+/** \addtogroup OptimizerRules
+ *  @{
+ */
+
+/**
+ * @brief Rule that applies to a physical plan to fuse a select node in 
the 
+ *build predicate of a hash join to the join.
+ */
+class FuseHashSelect : public TopDownRule {
+ public:
+/**
+ * @brief Constructor.
+ */
+FuseHashSelect() {}
+
+~FuseHashSelect() override {}
+
+std::string getName() const override {
+  return "FuseHashSelect";
+}
+
+ protected:
+physical::PhysicalPtr applyToNode(const physical::PhysicalPtr ) 
override;
+
+ private:
+DISALLOW_COPY_AND_ASSIGN(FuseHashSelect);
+};
+
+}  // namespace optimizer
+}  // namespace quickstep
+
+#endif /* QUICKSTEP_QUERY_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_ */
--- End diff --

The new style is `#endif  // 
QUICKSTEP_QUERY_OPTIMIZER_RULES_FUSE_HASH_SELECT_HPP_`.


---

[GitHub] incubator-quickstep issue #299: QUICKSTEP-104 Fix the problem that LineReade...

[zuyu]

Github user zuyu commented on the issue:

https://github.com/apache/incubator-quickstep/pull/299
  
LGTM!


---