[GitHub] incubator-quickstep pull request #301: Bug fix in LockManager loop
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/301#discussion_r141387707
--- Diff: transaction/LockManager.cpp ---
@@ -81,15 +81,17 @@ void LockManager::run() {
CHECK(releaseAllLocks(request.getTransactionId()))
<< "Unexpected condition occured.";
-} else if (acquireLock(request.getTransactionId(),
--- End diff --
Good catch!
---
[GitHub] incubator-quickstep issue #233: QUICKSTEP-88 Topological sort functionality ...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/233 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #233: QUICKSTEP-88 Topological sort functio...
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/233#discussion_r112284625
--- Diff: utility/tests/DAG_unittest.cpp ---
@@ -490,6 +490,88 @@ TEST(DAGTest, LinkMetadataBoolTest) {
EXPECT_FALSE(dag_.getLinkMetadata(1, 0));
}
+TEST(DAGTest, TopoSortTest) {
+ const int kNodeSize = 5;
+ DAG<DummyPayload, int> dag_;
+
+ for (int node_index = 0; node_index < kNodeSize; ++node_index) {
+ASSERT_EQ(static_cast(node_index),
+ dag_.createNode(new DummyPayload(node_index)));
+ }
+
+ /*
+ *0
+ * / \
+ * v v
+ * 1 2
+ * \ /
+ *v
+ *3
+ *|
+ *v
+ *4
+ *
+ */
+
+ dag_.createLink(0, 1, 2);
+ dag_.createLink(0, 2, 1);
+ dag_.createLink(1, 3, 1);
+ dag_.createLink(2, 3, 1);
+ dag_.createLink(3, 4, 1);
+
+ vector<DAG<DummyPayload, int>::size_type_nodes> sorted_list =
+ dag_.getTopologicalSorting();
+ std::unordered_map<DAG<DummyPayload, int>::size_type_nodes, bool>
node_exists;
+ // First check if the ordering is a legal sequence of nodes, i.e. every
node
+ // appears exactly once.
+ for (auto node_id = 0u; node_id < dag_.size(); ++node_id) {
+node_exists[node_id] = false;
+ }
+ for (auto i : sorted_list) {
+node_exists[i] = true;
+ }
+ for (auto node_id = 0u; node_id < dag_.size(); ++node_id) {
+ASSERT_TRUE(node_exists[node_id]);
+ }
+ // We apply the following condition for verifying if we have obtained a
valid
+ // topological sorting.
+ // If there is an edge i->j between two nodes i and j, then i comes
before j
+ // in the topologically sorted order.
+ // We use a map to store the position of a given node in the sorted
order.
+ // Key = node ID, value = position of the node in the sorted order.
+ std::unordered_map<std::size_t, std::size_t> position_in_sorted_order;
+ for (std::size_t i = 0; i < sorted_list.size(); ++i) {
+position_in_sorted_order[sorted_list[i]] = i;
+ }
+ std::vector<std::tuple<std::size_t, std::size_t>> edges;
+ // Populate the list of edges.
+ edges.emplace_back(0, 1);
+ edges.emplace_back(0, 2);
+ edges.emplace_back(1, 3);
+ edges.emplace_back(2, 3);
+ edges.emplace_back(3, 4);
+ for (auto curr_edge : edges) {
--- End diff --
I suppose we need to check each reachable pair instead of checking each
pair that has an edge between them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #233: QUICKSTEP-88 Topological sort functio...
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/233#discussion_r112069450
--- Diff: utility/tests/DAG_unittest.cpp ---
@@ -490,6 +490,51 @@ TEST(DAGTest, LinkMetadataBoolTest) {
EXPECT_FALSE(dag_.getLinkMetadata(1, 0));
}
+TEST(DAGTest, TopoSortTest) {
--- End diff --
One way to check the correctness is by first creating `reachable` mapping,
and then checking for each node `a`, for each node `b` that come after the node
`a` in `topological_sorted_list`, there must be no `reachable` path from `b`
to `a`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #233: QUICKSTEP-88 Topological sort functio...
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/233#discussion_r112067899
--- Diff: utility/DAG.hpp ---
@@ -489,6 +495,40 @@ bool DAG<T, LinkMetadataT>::hasCycleHelper(const
typename DAG<T, LinkMetadataT>:
return false;
}
+template
+std::vector::size_type_nodes> DAG<T,
LinkMetadataT>::getTopologicalSorting() const {
+ // We implement "Kahn's algorithm" for the sorting.
+ DCHECK(!hasCycle());
+ std::unique_ptr<std::vector::size_type_nodes>>
+ sorted_list(new std::vector());
+ sorted_list->reserve(this->size());
+ // Key = node ID, value = # incoming edges for this node.
+ // NOTE(harshad) - We modify the "values" in this map as we go along.
+ std::unordered_map::size_type_nodes,
std::size_t> num_incoming_edges;
+ std::queue::size_type_nodes>
nodes_with_no_children;
+ for (auto node_id = 0u; node_id < this->size(); ++node_id) {
+if (nodes_[node_id].getDependencies().empty()) {
+ nodes_with_no_children.emplace(node_id);
+}
+num_incoming_edges[node_id] = nodes_[node_id].getDependencies().size();
+ }
+ while (!nodes_with_no_children.empty()) {
--- End diff --
I think this is the place where Kahn's algorithm starts to work. Can you
separate methods into logical pieces with comments or by refactoring?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #193: Use partitioned aggregation for single-funct...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/193 Looks good to me. I am merging with master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #190: Use BitVectorExactFilter as LIPFilter implem...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/190 It looks good to me. Merging. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #164: Added DAGAnalyzer and Pipeline classe...
Github user hakanmemisoglu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/164#discussion_r96951793 --- Diff: query_execution/DAGAnalyzer.hpp --- @@ -0,0 +1,100 @@ +/** + * 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_DAG_ANALYZER_HPP_ +#define QUICKSTEP_QUERY_EXECUTION_DAG_ANALYZER_HPP_ + +#include +#include + --- End diff -- We need `#include ` here. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #153: QUICKSTEP-65 Fix Quickstep build failure on ...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/153 It is also looking good to me. Merging. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #129: Partial inserts
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/129 @cramja I had the same problem, you are probably trying to push to the read-only Github repo. [Here is the link for the workflow.](https://cwiki.apache.org/confluence/display/QUICKSTEP/Workflow+For+Committers) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #126: QUICKSTEP-41 Automatically pin workers to CP...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/126 It is merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #109: Refactored SplitRowStore bulk insertion
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/109 Hi @cramja, the problem might be related lambda capture arguments. GCC 5 was giving the same problems if you use general capture by reference [&]. You can try to give specific reference parameters that is used in lambda function, such as [, ]. It should fix the problem. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #98: QUICKSTEP-53 DateLit improvements
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/98 I will close this PR since an issue in the compression code should be handled first. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #98: QUICKSTEP-53 DateLit improvements
Github user hakanmemisoglu closed the pull request at: https://github.com/apache/incubator-quickstep/pull/98 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #98: QUICKSTEP-53 DateLit improvements
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/98#discussion_r77749586
--- Diff: types/DatetimeLit.hpp ---
@@ -51,53 +54,70 @@ struct DateLit {
+ 1 // -
+ 2; // Day
+ // Years should be between [-kMaxYear, +kMaxYear] inclusive both end.
+ static constexpr std::int32_t kMaxYear = 9;
+ static constexpr std::uint8_t kBitsNeededForDay = 5u;
+ static constexpr std::uint8_t kBitsNeededForMonth = 4u;
+
static DateLit Create(const std::int32_t _year,
const std::uint8_t _month,
const std::uint8_t _day) {
DateLit date;
-date.year = _year;
-date.month = _month;
-date.day = _day;
+// Normalize year by adding kMaxYear value, because we try to
+// encode signed year value into an unsigned integer.
--- End diff --
Hi @hbdeshmukh
In the code, the leap year check and other checks such whether the year is
valid or not, are done within `printValueToString() `and
`parseValueFromString() `. These methods use `yearField()`, `monthField()`,
`dayField()` to decode the real number from the representation. I think it
should be OK.
I did not understand actually why it should provide:
> if _year is a leap year, is there a guarantee that _year + kMaxYear is
also a leap year?
Or do I miss some other leap year checks done in the code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #98: QUICKSTEP-53 DateLit improvements
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/98 @zuyu This will be merged after @hbdeshmukh's changes. Since we are use rebasing, I don't think it will be a problem. For the efficiency part: 1-) Using new comparison operators obviously are faster than old counterparts using if else statements. (Of course the overall effect might not be fast as we expected in the TPCH queries.) Thank you. 2-) No matter why TypedValue is used in query processing , the cutting the size of struct representation by 50% improves the memory usage: a-) A page can contain 50% more values. b-) Recent x86 architectures favor using the values 32 bits or 64 bits, not 48 bits. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #98: QUICKSTEP-53 DateLit improvements
GitHub user hakanmemisoglu opened a pull request: https://github.com/apache/incubator-quickstep/pull/98 QUICKSTEP-53 DateLit improvements This PR changes DateLit represantation: - Encode year, month and day information into an u32 field instead of keeping them separate in three different fields. The change reduces the size of DateLit from 48 bits to 32 bits. - With the internal representation change, we changed the comparison operators' implementation. They now use one unsigned integer comparison for each method. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-quickstep date-representation Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/98.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 #98 commit 3c8708dfd0987b3491bcabb94e77dd95ea0aea10 Author: Harshad Deshmukh <hbdeshm...@apache.org> Date: 2016-08-29T19:03:52Z Separate Date type from Datetime type. - Beginning from the parser to the operator execution level, treat Date type sepearately than the Datetime type. - Provide support for the extract function when applied on the Date type, implemented in the DateExtractOperation. - Modified the tests. commit 998994eee67774d24b4a31a8aea3b1924b0e04ea Author: Hakan Memisoglu <hakanmemiso...@gmail.com> Date: 2016-09-02T19:53:47Z New representation and comparison operators for DateLit. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #37: DO NOT MERGE: QUICKSTEP-6: Decimal typ...
Github user hakanmemisoglu closed the pull request at: https://github.com/apache/incubator-quickstep/pull/37 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #73: Move hash join's probe and build node ...
Github user hakanmemisoglu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/73#discussion_r73118755
--- Diff: query_optimizer/rules/SwapProbeBuild.cpp ---
@@ -0,0 +1,72 @@
+#include "query_optimizer/rules/SwapProbeBuild.hpp"
+
+#include
+#include
+
+#include "query_optimizer/cost_model/StarSchemaSimpleCostModel.hpp"
+#include "query_optimizer/expressions/AttributeReference.hpp"
+#include "query_optimizer/physical/HashJoin.hpp"
+#include "query_optimizer/physical/PatternMatcher.hpp"
+#include "query_optimizer/physical/Physical.hpp"
+#include "query_optimizer/physical/TopLevelPlan.hpp"
+#include "query_optimizer/rules/Rule.hpp"
+
+
+namespace quickstep {
+namespace optimizer {
+
+P::PhysicalPtr SwapProbeBuild::applyToNode(const P::PhysicalPtr ) {
+ P::HashJoinPtr hash_join;
+
+ if (P::SomeHashJoin::MatchesWithConditionalCast(input, _join)) {
+P::PhysicalPtr left = hash_join->left();
+P::PhysicalPtr right = hash_join->right();
+
+P::TopLevelPlanPtr top_level;
+if (P::SomeTopLevelPlan::MatchesWithConditionalCast(input,
_level)) {
--- End diff --
@jianqiao , Thanks for the catch.
> In fact, BottomUpRule does not provide a place for initializing
cost_model_.
It is the reason why, I have implemented initialization of cost model in
"applyToNode".
I will introduce init(const P::PhysicalPtr _level_plan) as you said.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #50: Fixed the time measurement from milli to micr...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/50 @hbdeshmukh, thank you for the notification. I have already followed the instructions. Actually the branch is not in original Apache repo, however it is accessible from GitHub. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep issue #50: Fixed the time measurement from milli to micr...
Github user hakanmemisoglu commented on the issue: https://github.com/apache/incubator-quickstep/pull/50 It seems alright. I am merging it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #47: QUICKSTEP-33: Fixed the bug in Numeric...
GitHub user hakanmemisoglu opened a pull request: https://github.com/apache/incubator-quickstep/pull/47 QUICKSTEP-33: Fixed the bug in NumericCast. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-quickstep quickstep-33 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/47.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 #47 commit d823657312cb9a897a0a8dd6dec324924462e007 Author: Hakan Memisoglu <hakanmemiso...@gmail.com> Date: 2016-06-29T19:01:13Z QUICKSTEP-33: Fixed the bug in NumericCast. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-quickstep pull request #37: QUICKSTEP-6: Decimal type
GitHub user hakanmemisoglu opened a pull request: https://github.com/apache/incubator-quickstep/pull/37 QUICKSTEP-6: Decimal type This request adds the simple Decimal type. The type does not accept any precision and scale argument for now. It takes default values 18 for precision and 2 for scale. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-quickstep decimal-type Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-quickstep/pull/37.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 #37 commit 5aa559310b017f03e9ef648980f47198ba2e6e9c Author: Hakan Memisoglu <hakanmemiso...@apache.org> Date: 2016-05-31T18:14:42Z New type for fixed precision number: Decimal. commit d71599fd124d9ddaf97150b52c7e701e4bcd121b Author: Hakan Memisoglu <hakanmemiso...@apache.org> Date: 2016-06-14T15:09:25Z New changes. commit 400d268b9d28e331707cc58094b9f0decc415d5c Author: Hakan Memisoglu <hakanmemiso...@apache.org> Date: 2016-06-15T18:16:18Z Make scale fixed to 10^2. commit 4358b6eddd63016f665f0774833ce1d0b81cbee5 Author: Hakan Memisoglu <hakanmemiso...@apache.org> Date: 2016-06-15T21:23:24Z Introduced comparator functors for Decimal type. commit 6b11fe0c214fc3d3d849f0916ba67f40fb37ffdd Author: Hakan Memisoglu <hakanmemiso...@apache.org> Date: 2016-06-16T17:03:13Z Fixed bug where the wrong operator is used. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
