[GitHub] incubator-quickstep issue #234: Add protobuf support for UNION ALL operator.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/234
  
LTM if comments are addressed.


---
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 #234: Add protobuf support for UNION ALL op...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/234#discussion_r112333485
  
--- Diff: relational_operators/UnionAllOperator.hpp ---
@@ -149,6 +151,11 @@ class UnionAllOperator : public RelationalOperator {
InsertDestination *output_destination,
const std::size_t relation_index);
 
+  // Create work order proto
--- End diff --

Does this require a full doxy comment?


---
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 #234: Add protobuf support for UNION ALL op...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/234#discussion_r11271
  
--- Diff: relational_operators/UnionAllOperator.cpp ---
@@ -128,14 +128,56 @@ bool UnionAllOperator::getAllWorkOrders(
 }
 
 bool UnionAllOperator::getAllWorkOrderProtos(WorkOrderProtosContainer* 
container) {
-  // TODO(tianrun): Add protobuf for UnionAllWorkOrder to support 
distributed mode.
-  LOG(FATAL) << "UnionAllOperator is not supported in distributed mode 
yet.";
-  return true;
+  if (!stored_generated_) {
+for (std::size_t relation_index = 0; relation_index < 
input_relations_.size(); ++relation_index) {
+  if (input_relations_are_stored_[relation_index]) {
+const std::vector _blocks = 
input_relations_block_ids_[relation_index];
+const relation_id relation = 
input_relations_[relation_index]->getID();
+const std::vector  = 
select_attribute_ids_[relation_index];
+for (const block_id block : all_blocks) {
+  container->addWorkOrderProto(createWorkOrderProto(block, 
relation, attributes), op_index_);
+}
+  }
+}
+stored_generated_ = true;
+  }
+
+  for (std::size_t relation_index = 0; relation_index < 
input_relations_.size(); ++relation_index) {
+if (!input_relations_are_stored_[relation_index]) {
+  const std::vector _blocks = 
input_relations_block_ids_[relation_index];
+  std::size_t num_generated = 
num_workorders_generated_[relation_index];
+  const relation_id relation = 
input_relations_[relation_index]->getID();
+  const std::vector  = 
select_attribute_ids_[relation_index];
+  while (num_generated < all_blocks.size()) {
+
container->addWorkOrderProto(createWorkOrderProto(all_blocks[num_generated], 
relation, attributes), op_index_);
+++num_generated;
+  }
+  num_workorders_generated_[relation_index] = num_generated;
+}
+  }
+  return stored_generated_ && done_feeding_input_relation_;
--- End diff --

Is `stored_generated_ && ...` redundant, as it will always be true 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 pull request #234: Add protobuf support for UNION ALL op...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/234#discussion_r112333067
  
--- Diff: relational_operators/UnionAllOperator.cpp ---
@@ -128,14 +128,56 @@ bool UnionAllOperator::getAllWorkOrders(
 }
 
 bool UnionAllOperator::getAllWorkOrderProtos(WorkOrderProtosContainer* 
container) {
-  // TODO(tianrun): Add protobuf for UnionAllWorkOrder to support 
distributed mode.
-  LOG(FATAL) << "UnionAllOperator is not supported in distributed mode 
yet.";
-  return true;
+  if (!stored_generated_) {
+for (std::size_t relation_index = 0; relation_index < 
input_relations_.size(); ++relation_index) {
--- End diff --

I'd suggest to refactor this into a small `generateStored (?)` method, 
because it seems like isolatable functionality.


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112325082
  
--- Diff: cli/LineReaderBuffered.hpp ---
@@ -0,0 +1,70 @@
+/**
+ * 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_CLI_LINE_READER_BUFFERED_HPP_
+#define QUICKSTEP_CLI_LINE_READER_BUFFERED_HPP_
+
+#include "cli/LineReader.hpp"
+
+namespace quickstep {
+
+class LineReaderBuffered : public LineReader {
+ public:
+  /**
+   * A line reader which accepts a string buffer.
+   * This does no IO and ignores any prompts given to the constructor.
+   * @param default_prompt Ignored.
+   * @param continue_prompt Ignored.
--- End diff --

how about:
```
  /**
   * @brief A line reader which accepts a string buffer.
   * Other line readers are ment to support some form of user interaction. 
This linereader does not and is intended for
   * programmer use- it does not print anything to stdout. Therefore it 
ignores any prompt strings given to the
   * inherited constructor.
   * @param default_prompt Not used by this line reader, but required by 
interface.
   * @param continue_prompt Not used by this line reader, but required by 
interface.
   */
```


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112324538
  
--- Diff: cli/LineReaderBuffered.hpp ---
@@ -0,0 +1,70 @@
+/**
+ * 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_CLI_LINE_READER_BUFFERED_HPP_
+#define QUICKSTEP_CLI_LINE_READER_BUFFERED_HPP_
+
+#include "cli/LineReader.hpp"
+
+namespace quickstep {
+
+class LineReaderBuffered : public LineReader {
--- End diff --

I agree, though I was trying to keep consistency with the 2 other LR's: 
LRLineNoise, LRDumb


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112324457
  
--- Diff: cli/LineReaderBuffered.cpp ---
@@ -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.
+ **/
+
+#include "cli/LineReaderBuffered.hpp"
+#include 
+
+namespace quickstep {
+
+LineReaderBuffered::LineReaderBuffered(const std::string _prompt,
+   const std::string _prompt)
+  : LineReader("", ""),
+buffer_empty_(true) { }
+
+LineReaderBuffered::LineReaderBuffered() : LineReader("", ""), 
buffer_empty_(true) { }
+
+std::string LineReaderBuffered::getLineInternal(const bool continuing) {
+  // This method is called when the leftover_ string is depleted.
+  buffer_empty_ = true;
+  return "";
+}
+
+}
+
+
+/*
+TEST(NetworkIOTest, TestLineReaderBuffered) {
+  // The buffered line reader is used exclusively by the NetworkIO.
+  LineReaderBuffered linereader;
+  EXPECT_TRUE(linereader.bufferEmpty());
+
+  std::string stmt_1 = "select * from foo;";
+  std::string stmts = stmt_1 + "select 1; select 2; quit;";
+  linereader.setBuffer(stmts);
+  ASSERT_FALSE(linereader.bufferEmpty());
+
+  std::vector stmts_vec;
+  int parsed_commands;
+  for(parsed_commands = 0;
+  parsed_commands < 10 && !linereader.bufferEmpty();
+  parsed_commands++) {
+std::string command = linereader.getNextCommand();
+if (command != "") {
+  stmts_vec.push_back(command);
+}
+  }
+
+  EXPECT_EQ(stmt_1, stmts_vec.front());
+  EXPECT_EQ(4, stmts_vec.size());
+  EXPECT_TRUE(linereader.bufferEmpty());
+}
+*/
--- End diff --

..well, actually I moved the commented out comments back into the unit 
test. Originally I had written this LRBuffered class, then removed it, placing 
the files somewhere safe. When I moved them back in, I forgot that I stashed 
the test in the file.


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112323095
  
--- Diff: cli/LineReaderBuffered.cpp ---
@@ -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.
+ **/
+
+#include "cli/LineReaderBuffered.hpp"
+#include 
+
+namespace quickstep {
+
+LineReaderBuffered::LineReaderBuffered(const std::string _prompt,
+   const std::string _prompt)
+  : LineReader("", ""),
+buffer_empty_(true) { }
+
+LineReaderBuffered::LineReaderBuffered() : LineReader("", ""), 
buffer_empty_(true) { }
+
+std::string LineReaderBuffered::getLineInternal(const bool continuing) {
+  // This method is called when the leftover_ string is depleted.
+  buffer_empty_ = true;
+  return "";
+}
+
+}
+
+
+/*
+TEST(NetworkIOTest, TestLineReaderBuffered) {
+  // The buffered line reader is used exclusively by the NetworkIO.
+  LineReaderBuffered linereader;
+  EXPECT_TRUE(linereader.bufferEmpty());
+
+  std::string stmt_1 = "select * from foo;";
+  std::string stmts = stmt_1 + "select 1; select 2; quit;";
+  linereader.setBuffer(stmts);
+  ASSERT_FALSE(linereader.bufferEmpty());
+
+  std::vector stmts_vec;
+  int parsed_commands;
+  for(parsed_commands = 0;
+  parsed_commands < 10 && !linereader.bufferEmpty();
+  parsed_commands++) {
+std::string command = linereader.getNextCommand();
+if (command != "") {
+  stmts_vec.push_back(command);
+}
+  }
+
+  EXPECT_EQ(stmt_1, stmts_vec.front());
+  EXPECT_EQ(4, stmts_vec.size());
+  EXPECT_TRUE(linereader.bufferEmpty());
+}
+*/
--- End diff --

Ah, I was stashing this the old fashion way... removed



---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[cramja]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112322839
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
+   * @note When the command is complete, commandComplete() should be 
called so that certain implementations can clean
+   *up state related to sending the command.
+   * @return A string containing a quickstep command.
+   */
+  virtual std::string nextCommand() = 0;
--- End diff --

Yes, I think this makes sense


---
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 #235: API to get total pending work orders ...

[asfgit]

Github user asfgit closed the pull request at:

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


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

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 #233: QUICKSTEP-88 Topological sort functionality ...

[hakanmemisoglu]

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

[hakanmemisoglu]

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 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::size_type_nodes> sorted_list =
+  dag_.getTopologicalSorting();
+  std::unordered_map::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 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> 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 #235: API to get total pending work orders ...

[hbdeshmukh]

GitHub user hbdeshmukh opened a pull request:

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

API to get total pending work orders for an operator

The total count includes normal and rebuild work orders.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/hbdeshmukh/incubator-quickstep 
get-total-workorders-wocontainer

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-quickstep/pull/235.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 #235


commit 9b655230898d2e2dfe988477ccf93e25dc96
Author: Harshad Deshmukh 
Date:   2017-04-19T18:31:08Z

API to get total pending work orders for an operator

- Total includes normal and rebuild work orders.




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

[hakanmemisoglu]

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

[hakanmemisoglu]

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::hasCycleHelper(const 
typename DAG:
   return false;
 }
 
+template 
+std::vector::size_type_nodes> DAG::getTopologicalSorting() const {
+  // We implement "Kahn's algorithm" for the sorting.
+  DCHECK(!hasCycle());
+  std::unique_ptr>
+  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 pull request #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112250274
  
--- Diff: cli/LineReaderBuffered.cpp ---
@@ -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.
+ **/
+
+#include "cli/LineReaderBuffered.hpp"
+#include 
+
+namespace quickstep {
+
+LineReaderBuffered::LineReaderBuffered(const std::string _prompt,
+   const std::string _prompt)
+  : LineReader("", ""),
+buffer_empty_(true) { }
+
+LineReaderBuffered::LineReaderBuffered() : LineReader("", ""), 
buffer_empty_(true) { }
+
+std::string LineReaderBuffered::getLineInternal(const bool continuing) {
+  // This method is called when the leftover_ string is depleted.
+  buffer_empty_ = true;
+  return "";
+}
+
+}
+
+
+/*
+TEST(NetworkIOTest, TestLineReaderBuffered) {
+  // The buffered line reader is used exclusively by the NetworkIO.
+  LineReaderBuffered linereader;
+  EXPECT_TRUE(linereader.bufferEmpty());
+
+  std::string stmt_1 = "select * from foo;";
+  std::string stmts = stmt_1 + "select 1; select 2; quit;";
+  linereader.setBuffer(stmts);
+  ASSERT_FALSE(linereader.bufferEmpty());
+
+  std::vector stmts_vec;
+  int parsed_commands;
+  for(parsed_commands = 0;
+  parsed_commands < 10 && !linereader.bufferEmpty();
+  parsed_commands++) {
+std::string command = linereader.getNextCommand();
+if (command != "") {
+  stmts_vec.push_back(command);
+}
+  }
+
+  EXPECT_EQ(stmt_1, stmts_vec.front());
+  EXPECT_EQ(4, stmts_vec.size());
+  EXPECT_TRUE(linereader.bufferEmpty());
+}
+*/
--- End diff --

Can you either uncomment the dead code or remove 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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112250845
  
--- Diff: cli/LineReaderBuffered.hpp ---
@@ -0,0 +1,70 @@
+/**
+ * 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_CLI_LINE_READER_BUFFERED_HPP_
+#define QUICKSTEP_CLI_LINE_READER_BUFFERED_HPP_
+
+#include "cli/LineReader.hpp"
+
+namespace quickstep {
+
+class LineReaderBuffered : public LineReader {
--- End diff --

Does ``BufferedLineReader`` sound a better name? 


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112249998
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
+   * @note When the command is complete, commandComplete() should be 
called so that certain implementations can clean
+   *up state related to sending the command.
+   * @return A string containing a quickstep command.
+   */
+  virtual std::string nextCommand() = 0;
+
+  /**
+   * Notifies the IO system that the previously acquired command is 
complete.
+   */
+  virtual void commandComplete() {}
+
+  /**
+   * Notifies the IO system that quickstep is shutting down.
+   */
+  virtual void shutdown() {}
--- End diff --

Similar to earlier, how about ``notifyShutdown``?


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112250621
  
--- Diff: cli/LineReaderBuffered.hpp ---
@@ -0,0 +1,70 @@
+/**
+ * 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_CLI_LINE_READER_BUFFERED_HPP_
+#define QUICKSTEP_CLI_LINE_READER_BUFFERED_HPP_
+
+#include "cli/LineReader.hpp"
+
+namespace quickstep {
+
+class LineReaderBuffered : public LineReader {
+ public:
+  /**
+   * A line reader which accepts a string buffer.
+   * This does no IO and ignores any prompts given to the constructor.
+   * @param default_prompt Ignored.
+   * @param continue_prompt Ignored.
--- End diff --

I don't quite understand the doxygen comments for the constructor 
arguments. Can you please elaborate? 


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112249915
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
+   * @note When the command is complete, commandComplete() should be 
called so that certain implementations can clean
+   *up state related to sending the command.
+   * @return A string containing a quickstep command.
+   */
+  virtual std::string nextCommand() = 0;
+
+  /**
+   * Notifies the IO system that the previously acquired command is 
complete.
+   */
+  virtual void commandComplete() {}
--- End diff --

How about ``notifyCommandComplete`` as a function name? In general, I try 
to name functions as a verb, so that their name can express what they are 
doing. 


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112249411
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
+   * @note When the command is complete, commandComplete() should be 
called so that certain implementations can clean
+   *up state related to sending the command.
+   * @return A string containing a quickstep command.
+   */
+  virtual std::string nextCommand() = 0;
+
+  /**
+   * Notifies the IO system that the previously acquired command is 
complete.
+   */
+  virtual void commandComplete() {}
+
+  /**
+   * Notifies the IO system that quickstep is shutting down.
--- End diff --

Missing the @ brief annotation


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112249378
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
+   * @note When the command is complete, commandComplete() should be 
called so that certain implementations can clean
+   *up state related to sending the command.
+   * @return A string containing a quickstep command.
+   */
+  virtual std::string nextCommand() = 0;
+
+  /**
+   * Notifies the IO system that the previously acquired command is 
complete.
--- End diff --

Missing the @ brief annotation


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112249266
  
--- Diff: cli/IOInterface.hpp ---
@@ -0,0 +1,61 @@
+/**
+ * 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_CLI_IO_INTERFACE_HPP_
+#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
+
+#include 
+
+/**
+ * Virtual base defines a generic, file-based interface around IO.
+ */
+class IOInterface {
+ public:
+  IOInterface() {}
+
+  /**
+   * @return A file handle for standard output.
+   */
+  virtual FILE* out() = 0;
+
+  /**
+   * @return A file handle for error output.
+   */
+  virtual FILE* err() = 0;
+
+  /**
+   * Requests a complete SQL command. This call may block until user input 
is given.
--- End diff --

Missing the @brief annotation 


---
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 #232: QUICKSTEP-87 Adds network cli interfa...

[hbdeshmukh]

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

https://github.com/apache/incubator-quickstep/pull/232#discussion_r112248871
  
--- Diff: cli/Flags.cpp ---
@@ -87,4 +87,15 @@ DEFINE_bool(preload_buffer_pool, false,
 "accepting queries (should also set --buffer_pool_slots to be "
 "large enough to accomodate the entire database).");
 
+static bool ValidatePort(const char *flagname, std::int32_t value) {
+  if (value > 0 && value < 65536) {
+return true;
+  }
+  std::cout << "Invalid value for --" << flagname << ": " << 
std::to_string(value) << std::endl;
--- End diff --

Should the error message also include the valid range of values? That might 
be helpful. 


---
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 #233: QUICKSTEP-88 Topological sort functionality ...

[hbdeshmukh]

Github user hbdeshmukh commented on the issue:

https://github.com/apache/incubator-quickstep/pull/233
  
Hi @hakanmemisoglu 

Can you please review this PR? Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---