Addressed commments from index team

Author: Shangjie Chen

src/catalog/catalog.cpp

@@ -29,18 +29,20 @@ AlterExecutor::AlterExecutor(const planner::AbstractPlan *node,
 // Initialize executor
 // Nothing to initialize for now
 bool AlterExecutor::DInit() {
-  LOG_TRACE("Initializing Alter Executer...");
+  LOG_TRACE("Initializing Alter Executor...");
 
   LOG_TRACE("Alter Executor initialized!");
   return true;
 }
 
 bool AlterExecutor::DExecute() {
   LOG_TRACE("Executing Alter...");
-  bool result = false;
+  bool result;
   const planner::AlterPlan &node = GetPlanNode<planner::AlterPlan>();
   auto current_txn = executor_context_->GetTransaction();
   AlterType type = node.GetAlterTableType();
+
+  // TODO: grab per-table lock before execution.
   switch (type) {
     case AlterType::RENAME:
       result = RenameColumn(node, current_txn);
@@ -65,13 +67,13 @@ bool AlterExecutor::RenameColumn(
   auto old_column_name = node.GetOldName();
 
   ResultType result = catalog::Catalog::GetInstance()->RenameColumn(
-      database_name, table_name, old_column_name, new_column_name, schema_name, txn);
+      database_name, table_name, old_column_name, new_column_name, schema_name,
+      txn);
   txn->SetResult(result);
 
   if (txn->GetResult() == ResultType::SUCCESS) {
     LOG_TRACE("Rename column succeeded!");
 
-    // TODO: Add on succeed logic if necessary
     executor_context_->num_processed = 1;
   } else {
     LOG_TRACE("Result is: %s", ResultTypeToString(txn->GetResult()).c_str());
@@ -133,8 +135,6 @@ bool AlterExecutor::AlterTable(const peloton::planner::AlterPlan &node,
       return false;
     } else {
       columns[i] = std::move(change_col);
-      // TODO: decide VARCHAR's size when change type
-      // if (change_pair.second == type::TypeId::VARCHAR) {}
     }
   }
 
@@ -168,13 +168,11 @@ bool AlterExecutor::AlterTable(const peloton::planner::AlterPlan &node,
     case ResultType::SUCCESS:
       LOG_TRACE("Alter table succeed!");
 
-      // TODO: Add on succeed logic if necessary
       executor_context_->num_processed = 1;
       break;
     case ResultType::FAILURE:
       LOG_TRACE("Alter table failed!");
 
-      // TODO: Add on failed logic if necessary
       break;
     default:
       LOG_TRACE("Result is: %s", ResultTypeToString(txn->GetResult()).c_str());

src/executor/alter_executor.cpp

@@ -89,7 +89,8 @@ class ColumnCatalog : public AbstractCatalog {
   bool DeleteColumn(oid_t table_oid, const std::string &column_name,
                     concurrency::TransactionContext *txn);
   bool DeleteColumns(oid_t table_oid, concurrency::TransactionContext *txn);
-  bool RenameColumn(oid_t database_oid, oid_t table_oid, const std::string &column_name,
+  bool RenameColumn(oid_t database_oid, oid_t table_oid,
+                    const std::string &column_name,
                     const std::string &new_name,
                     concurrency::TransactionContext *txn);
 

src/include/catalog/column_catalog.h

@@ -51,14 +51,14 @@ class TransactionContext : public Printable {
 
  public:
   TransactionContext(const size_t thread_id, const IsolationLevelType isolation,
-              const cid_t &read_id);
+                     const cid_t &read_id);
 
   TransactionContext(const size_t thread_id, const IsolationLevelType isolation,
-              const cid_t &read_id, const cid_t &commit_id);
- 
+                     const cid_t &read_id, const cid_t &commit_id);
+
   TransactionContext(const size_t thread_id, const IsolationLevelType isolation,
-              const cid_t &read_id, const cid_t &commit_id, 
-              const size_t read_write_set_size);
+                     const cid_t &read_id, const cid_t &commit_id,
+                     const size_t read_write_set_size);
 
   /**
    * @brief      Destroys the object.
@@ -126,8 +126,9 @@ class TransactionContext : public Printable {
    *
    * @return     The query strings.
    */
-  inline const std::vector<std::string>& GetQueryStrings() const {
-                                                      return query_strings_; }
+  inline const std::vector<std::string> &GetQueryStrings() const {
+    return query_strings_;
+  }
 
   /**
    * @brief      Sets the commit identifier.
@@ -142,7 +143,7 @@ class TransactionContext : public Printable {
    * @param[in]  epoch_id  The epoch identifier
    */
   inline void SetEpochId(const eid_t epoch_id) { epoch_id_ = epoch_id; }
-  
+
   /**
    * @brief      Sets the timestamp.
    *
@@ -155,18 +156,18 @@ class TransactionContext : public Printable {
    *
    * @param[in]  query_string  The query string
    */
-  inline void AddQueryString(const char* query_string) {
+  inline void AddQueryString(const char *query_string) {
     query_strings_.push_back(std::string(query_string));
   }
 
   void RecordCreate(oid_t database_oid, oid_t table_oid, oid_t index_oid) {
-    rw_object_set_.push_back(std::make_tuple(database_oid, table_oid,
-                                index_oid, DDLType::CREATE));
+    rw_object_set_.push_back(
+        std::make_tuple(database_oid, table_oid, index_oid, DDLType::CREATE));
   }
 
   void RecordDrop(oid_t database_oid, oid_t table_oid, oid_t index_oid) {
-    rw_object_set_.push_back(std::make_tuple(database_oid, table_oid,
-                                index_oid, DDLType::DROP));
+    rw_object_set_.push_back(
+        std::make_tuple(database_oid, table_oid, index_oid, DDLType::DROP));
   }
 
   void RecordRead(const ItemPointer &);
@@ -177,10 +178,18 @@ class TransactionContext : public Printable {
 
   void RecordInsert(const ItemPointer &);
 
+  /**
+   * @brief Record drop of a data table.
+   * @param table the table dropped.
+   */
   void RecordDropTable(storage::DataTable *table) {
     dropped_tables.push_back(table);
   }
 
+  /**
+   * @brief Get the dropped tables.
+   * @return vector of dropped tables.
+   */
   std::vector<storage::DataTable *> &GetDroppedTables() {
     return dropped_tables;
   }
@@ -338,8 +347,8 @@ class TransactionContext : public Printable {
   ReadWriteSet rw_set_;
   CreateDropSet rw_object_set_;
 
-  /** 
-   * this set contains data location that needs to be gc'd in the transaction. 
+  /**
+   * this set contains data location that needs to be gc'd in the transaction.
    */
   std::shared_ptr<GCSet> gc_set_;
   std::shared_ptr<GCObjectSet> gc_object_set_;

src/include/concurrency/transaction_context.h

@@ -2,59 +2,66 @@
 //
 //                         Peloton
 //
-// alter_function_statement.h
+// alter_statement.h
 //
-// Identification: src/include/parser/alter_function_statement.h
+// Identification: src/include/parser/alter_statement.h
 //
-// Copyright (c) 2015-17, Carnegie Mellon University Database Group
+// Copyright (c) 2015-18, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
 #pragma once
 
-#include "parser/sql_statement.h"
 #include "common/sql_node_visitor.h"
+#include "parser/sql_statement.h"
 #include "parser/create_statement.h"
 
 namespace peloton {
 namespace parser {
+
 /**
  * @struct AlterTableStatement
  * @brief Represents "ALTER TABLE add column COLUMN_NAME COLUMN_TYPE"
- * TODO: add implementation of AlterTableStatement
  */
 class AlterTableStatement : public TableRefStatement {
  public:
   enum class AlterTableType { INVALID = 0, ALTER = 1, RENAME = 2 };
+
   AlterTableStatement(AlterTableType type)
       : TableRefStatement(StatementType::ALTER),
         type(type),
         oldName(nullptr),
         newName(nullptr) {
-    dropped_names =
-        type == AlterTableType::RENAME ? nullptr : (new std::vector<char *>);
-    added_columns = type == AlterTableType::RENAME
-                        ? nullptr
-                        : (new std::vector<std::unique_ptr<ColumnDefinition>>);
-    changed_type_columns =
-        type == AlterTableType::RENAME
-            ? nullptr
-            : (new std::vector<std::unique_ptr<ColumnDefinition>>);
+    if (type == AlterTableType::RENAME) {
+      dropped_names = nullptr;
+      added_columns = nullptr;
+      changed_type_columns = nullptr;
+    } else {
+      dropped_names = new std::vector<char *>;
+      added_columns = new std::vector<std::unique_ptr<ColumnDefinition>>;
+      changed_type_columns = new std::vector<std::unique_ptr<ColumnDefinition>>;
+    }
   }
 
   virtual ~AlterTableStatement() {
     if (added_columns != nullptr) {
       delete added_columns;
     }
     if (dropped_names != nullptr) {
-      for (auto name : *dropped_names) delete name;
+      for (auto name : *dropped_names) {
+        delete name;
+      }
       delete dropped_names;
     }
     if (changed_type_columns != nullptr) {
       delete changed_type_columns;
     }
-    if (oldName) delete oldName;
-    if (newName) delete newName;
+    if (oldName) {
+      delete oldName;
+    }
+    if (newName) {
+      delete newName;
+    }
   }
 
   virtual void Accept(SqlNodeVisitor *v) override { v->Visit(this); }
@@ -68,10 +75,11 @@ class AlterTableStatement : public TableRefStatement {
   std::vector<std::unique_ptr<ColumnDefinition>> *added_columns;
 
   std::vector<std::unique_ptr<ColumnDefinition>> *changed_type_columns;
+
   // the name that needs to be changed
   char *oldName;
   char *newName;
 };
 
 }  // End parser namespace
-}  // End peloton namespace
+}  // End peloton namespace

src/include/parser/alter_statement.h

@@ -14,8 +14,8 @@
 
 // This is just for convenience
 
-#include "analyze_statement.h"
 #include "alter_statement.h"
+#include "analyze_statement.h"
 #include "copy_statement.h"
 #include "create_function_statement.h"
 #include "create_statement.h"

src/include/parser/statements.h

@@ -6,13 +6,15 @@
 //
 // Identification: src/include/parser/alter_plan.h
 //
-// Copyright (c) 2015-17, Carnegie Mellon University Database Group
+// Copyright (c) 2015-18, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
 #pragma once
-#include "planner/abstract_plan.h"
+
 #include "parser/alter_statement.h"
+#include "planner/abstract_plan.h"
+
 namespace peloton {
 
 namespace parser {
@@ -26,8 +28,8 @@ class DataTable;
 }
 
 namespace planner {
+
 /** @brief The plan used for altering
- *  TODO: adding support for add/drop column
  */
 class AlterPlan : public AbstractPlan {
  public:
@@ -39,11 +41,11 @@ class AlterPlan : public AbstractPlan {
       const std::unique_ptr<catalog::Schema> &added_columns,
       const std::unique_ptr<catalog::Schema> &changed_type_columns,
       AlterType a_type);
+
   explicit AlterPlan(const std::string &database_name,
-                     const std::string &table_name,
-                     const std::vector<std::string> &old_names,
-                     const std::vector<std::string> &new_names,
-                     AlterType a_type);
+                     const std::string &table_name, const std::string &old_name,
+                     const std::string &new_name, AlterType a_type);
+
   explicit AlterPlan(parser::AlterTableStatement *parse_tree);
 
   virtual ~AlterPlan() {}
@@ -61,11 +63,12 @@ class AlterPlan : public AbstractPlan {
   std::unique_ptr<AbstractPlan> Copy() const {
     switch (this->type) {
       case AlterType::ALTER:
-        return std::unique_ptr<AbstractPlan>(new AlterPlan(
-            database_name, table_name, dropped_columns, added_columns, changed_type_columns, type));
+        return std::unique_ptr<AbstractPlan>(
+            new AlterPlan(database_name, table_name, dropped_columns,
+                          added_columns, changed_type_columns, type));
       case AlterType::RENAME:
         return std::unique_ptr<AbstractPlan>(new AlterPlan(
-            database_name, table_name, old_names_, new_names_, type));
+            database_name, table_name, old_name_, new_name_, type));
       default:
         LOG_ERROR("Not supported Copy of Alter type yet");
         return nullptr;
@@ -84,24 +87,24 @@ class AlterPlan : public AbstractPlan {
     return dropped_columns;
   }
 
-  const std::unique_ptr<catalog::Schema> &
-  GetChangedTypeColumns() const {
+  const std::unique_ptr<catalog::Schema> &GetChangedTypeColumns() const {
     return changed_type_columns;
   };
 
   AlterType GetAlterTableType() const { return type; }
 
   // function used for rename statement
-  std::string GetOldName() const { return this->old_names_[0]; }
+  std::string GetOldName() const { return this->old_name_; }
 
   // function used for rename statement
-  std::string GetNewName() const { return this->new_names_[0]; }
+  std::string GetNewName() const { return this->new_name_; }
 
   // return true if the alter plan is rename statement
   bool IsRename() const { return this->type == AlterType::RENAME; }
 
   // return schema name
   std::string GetSchemaName() const { return this->schema_name; }
+
  private:
   // Target Table
   storage::DataTable *target_table_ = nullptr;
@@ -121,8 +124,8 @@ class AlterPlan : public AbstractPlan {
   // changed-type columns, define the column you want to change type
   std::unique_ptr<catalog::Schema> changed_type_columns;
   // used for store rename function data
-  std::vector<std::string> old_names_;
-  std::vector<std::string> new_names_;
+  std::string old_name_;
+  std::string new_name_;
 
   // Check to either AlterTable Table, INDEX or Rename
   AlterType type;

src/include/planner/alter_plan.h

@@ -6,7 +6,7 @@
 //
 // Identification: src/optimizer/optimizer.cpp
 //
-// Copyright (c) 2015-16, Carnegie Mellon University Database Group
+// Copyright (c) 2015-18, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
@@ -147,7 +147,6 @@ unique_ptr<planner::AbstractPlan> Optimizer::HandleDDLStatement(
   auto stmt_type = tree->GetType();
   switch (stmt_type) {
     case StatementType::ALTER: {
-      // TODO (shilun) adding support of Alter
       LOG_TRACE("Adding Alter Plan");
       unique_ptr<planner::AbstractPlan> alter_plan(
           new planner::AlterPlan((parser::AlterTableStatement *)tree));