Merge pull request #10 from sxzh93/address-comment

Addressed comments in test and catalog;
Remove LOG_LEVEL change;
Change change column parameter.
Fix compile error.

Author: DeanChensj

script/testing/junit/AlterTableTest.java

@@ -66,7 +66,7 @@ public void Teardown() throws SQLException {
      * Insert 1 tuple, rename the column, and see if the change is visible.
      */
     @Test
-    public void test_RenameColumn_1() throws SQLException {
+    public void test_RenameCol_Base() throws SQLException {
         conn.createStatement().execute(SQL_RENAME_COLUMN);
         ResultSet rs = conn.createStatement().executeQuery(SQL_SELECT_STAR);
         rs.next();
@@ -80,7 +80,7 @@ public void test_RenameColumn_1() throws SQLException {
      * Rename a column that does not exists, should throw exception
      */
     @Test
-    public void test_RenameColumn_2() throws SQLException {
+    public void test_RenameCol_NotExist() throws SQLException {
         String sql = "ALTER TABLE foo RENAME a to b;";
 
         // Old column does not exist
@@ -89,10 +89,10 @@ public void test_RenameColumn_2() throws SQLException {
     }
 
     /**
-     * Rename a column that does not exists, should throw exception
+     * Rename a column to a name that already exists, should throw exception
      */
     @Test
-    public void test_RenameColumn_3() throws SQLException {
+    public void test_RenameCol_Exist() throws SQLException {
         String sql = "ALTER TABLE foo RENAME year to id;";
 
         // New column already exists
@@ -104,7 +104,7 @@ public void test_RenameColumn_3() throws SQLException {
      * Two transactions try to rename at the same time, should throw exception
      */
     @Test
-    public void test_RenameColumn_4() throws SQLException {
+    public void test_RenameCol_Concurrent() throws SQLException {
         conn.setAutoCommit(false);
         conn2.setAutoCommit(false);
 
@@ -118,8 +118,13 @@ public void test_RenameColumn_4() throws SQLException {
     }
 
 //    The following tests are currently broken.
+//
+//    /**
+//     * 2 transactions, t2 reads the table between t1 executes the rename
+//     * and commits the changes.
+//     */
 //    @Test
-//    public void test_RenameColumn_5() throws SQLException {
+//    public void test_RenameCol_ReadBeforeCommit() throws SQLException {
 //        conn.setAutoCommit(false);
 //        conn2.setAutoCommit(false);
 //
@@ -138,11 +143,11 @@ public void test_RenameColumn_4() throws SQLException {
 //    }
 //
 //    /**
-//     * 2 transactions, t2 read the table before and after t1 change the column
-//     * name and should not see the changes.
+//     * 2 transactions, t2 reads the table before and after t1 changes the
+//     * column name and should not see the changes.
 //     */
 //    @Test
-//    public void test_RenameColumn_6() throws SQLException {
+//    public void test_RenameCol_ReadBeforeAndAfterCommit() throws SQLException {
 //        conn.setAutoCommit(false);
 //        conn2.setAutoCommit(false);
 //
@@ -164,9 +169,12 @@ public void test_RenameColumn_4() throws SQLException {
 //        assertNoMoreRows(rs_2);
 //        conn2.commit();
 //    }
-
+//
+//    /**
+//     * 2 transactions, t2 reads the table after t1 commits the changes.
+//     */
 //    @Test
-//    public void test_RenameColumn_7() throws SQLException {
+//    public void test_RenameCol_ReadAfterCommit() throws SQLException {
 //        conn.setAutoCommit(false);
 //        conn2.setAutoCommit(false);
 //

src/catalog/catalog.cpp

@@ -791,7 +791,14 @@ std::shared_ptr<TableCatalogObject> Catalog::GetTableObject(
 //===--------------------------------------------------------------------===//
 // ALTER TABLE
 //===--------------------------------------------------------------------===//
-/* Helper function for alter table, called internally
+
+/**
+ * @brief Helper function for alter table, called internally
+ * @param database_oid database to which the table belongs to
+ * @param table_oid table to which the column belongs to
+ * @param new_schema the new table schema
+ * @param txn the transaction Context
+ * @return TransactionContext ResultType(SUCCESS or FAILURE)
  */
 ResultType Catalog::AlterTable(
     UNUSED_ATTRIBUTE oid_t database_oid, UNUSED_ATTRIBUTE oid_t table_oid,
@@ -803,14 +810,33 @@ ResultType Catalog::AlterTable(
   return ResultType::SUCCESS;
 }
 
+/**
+ * @brief Add new columns to the table.
+ * @param database_name database to which the table belongs to
+ * @param table_name table to which the column belongs to
+ * @param columns the column to be added
+ * @param txn the transaction Context
+ * @return TransactionContext ResultType(SUCCESS or FAILURE)
+ *
+ */
 ResultType Catalog::AddColumn(
     UNUSED_ATTRIBUTE const std::string &database_name,
     UNUSED_ATTRIBUTE const std::string &table_name,
     UNUSED_ATTRIBUTE const std::vector<std::string> &columns,
     UNUSED_ATTRIBUTE concurrency::TransactionContext *txn) {
+  // TODO: perform ADD Operation
   return ResultType::SUCCESS;
 }
 
+/**
+ * @brief Drop the column from the table.
+ * @param database_name database to which the table belongs to
+ * @param table_name table to which the columns belong to
+ * @param columns the columns to be dropped
+ * @param txn the transaction Context
+ * @return TransactionContext ResultType(SUCCESS or FAILURE)
+ */
+
 ResultType Catalog::DropColumn(
     const std::string &database_name,
     const std::string &table_name,
@@ -829,21 +855,29 @@ ResultType Catalog::DropColumn(
   return ResultType::SUCCESS;
 }
 
-ResultType Catalog::ChangeColumnName(const std::string &database_name,
-                                     const std::string &table_name,
-                                     const std::vector<std::string> &old_names,
-                                     const std::vector<std::string> &names,
-                                     concurrency::TransactionContext *txn) {
+/**
+ * @brief Change the column name in the catalog.
+ * @param database_name database to which the table belongs to
+ * @param table_name table to which the column belongs to
+ * @param columns the column to be dropped
+ * @param txn the transaction Context
+ * @return TransactionContext ResultType(SUCCESS or FAILURE)
+ */
+ResultType Catalog::RenameColumn(const std::string &database_name,
+                                 const std::string &table_name,
+                                 const std::string &old_name,
+                                 const std::string &new_name,
+                                 concurrency::TransactionContext *txn) {
   if (txn == nullptr) {
     throw CatalogException("Change Column requires transaction.");
   }
 
-  if (old_names.size() == 0 || names.size() == 0) {
-    throw CatalogException("No names are given.");
+  if (new_name.size() == 0) {
+    throw CatalogException("Name can not be empty string.");
   }
 
-  LOG_TRACE("Change Column Name %s to %s", old_names[0].c_str(),
-            names[0].c_str());
+  LOG_TRACE("Change Column Name %s to %s", old_name.c_str(),
+            new_name.c_str());
 
   try {
     // Get table from the name
@@ -854,23 +888,23 @@ ResultType Catalog::ChangeColumnName(const std::string &database_name,
     // Currently we only support change the first column name!
 
     // Check the validity of old name and the new name
-    oid_t columnId = schema->GetColumnID(names[0]);
+    oid_t columnId = schema->GetColumnID(new_name);
     if (columnId != INVALID_OID) {
       throw CatalogException("New column already exists in the table.");
     }
-    columnId = schema->GetColumnID(old_names[0]);
+    columnId = schema->GetColumnID(old_name);
     if (columnId == INVALID_OID) {
       throw CatalogException("Old column does not exist in the table.");
     }
 
     // Change column name in the global schema
-    schema->ChangeColumnName(columnId, names[0]);
+    schema->ChangeColumnName(columnId, new_name);
 
     // Change cached ColumnCatalog
     oid_t table_oid = Catalog::GetInstance()
                           ->GetTableObject(database_name, table_name, txn)
                           ->GetTableOid();
-    catalog::ColumnCatalog::GetInstance()->DeleteColumn(table_oid, old_names[0],
+    catalog::ColumnCatalog::GetInstance()->DeleteColumn(table_oid, old_name,
                                                         txn);
     auto new_column = schema->GetColumn(columnId);
     catalog::ColumnCatalog::GetInstance()->InsertColumn(

src/executor/alter_executor.cpp

@@ -35,7 +35,7 @@ bool AlterExecutor::DInit() {
 }
 
 bool AlterExecutor::DExecute() {
-  LOG_TRACE("Executing Drop...");
+  LOG_TRACE("Executing Alter...");
   bool result = false;
   if (!isAlter_) {
     const planner::AlterPlan &node = GetPlanNode<planner::AlterPlan>();
@@ -66,16 +66,15 @@ bool AlterExecutor::RenameColumn(
   auto new_column_name = node.GetNewName();
   auto old_column_name = node.GetOldName();
 
-  std::vector<std::string> old_names = {old_column_name};
-  std::vector<std::string> names = {new_column_name};
-  ResultType result = catalog::Catalog::GetInstance()->ChangeColumnName(
-      database_name, table_name, old_names, names, txn);
+  ResultType result = catalog::Catalog::GetInstance()->RenameColumn(
+      database_name, table_name, old_column_name, new_column_name, txn);
   txn->SetResult(result);
 
   if (txn->GetResult() == ResultType::SUCCESS) {
     LOG_TRACE("Rename column succeeded!");
 
-    // TODO Add on succeed logic if necessary
+    // TODO: Add on succeed logic if necessary
+    executor_context_->num_processed = 1;
   } else {
     LOG_TRACE("Result is: %s", ResultTypeToString(txn->GetResult()).c_str());
   }
@@ -95,7 +94,8 @@ bool AlterExecutor::DropColumn(const peloton::planner::AlterPlan &node,
   if (txn->GetResult() == ResultType::SUCCESS) {
     LOG_TRACE("Drop column succeed!");
 
-    // TODO Add on succeed logic if necessary
+    // TODO: Add on succeed logic if necessary
+    executor_context_->num_processed = 1;
   } else {
     LOG_TRACE("Result is: %s", ResultTypeToString(txn->GetResult()).c_str());
   }

src/include/catalog/catalog.h

@@ -194,11 +194,11 @@ class Catalog {
                         const std::vector<std::string> &columns,
                         concurrency::TransactionContext *txn);
 
-  ResultType ChangeColumnName(const std::string &database_name,
-                              const std::string &table_name,
-                              const std::vector<std::string> &old_columns,
-                              const std::vector<std::string> &names,
-                              concurrency::TransactionContext *txn);
+  ResultType RenameColumn(const std::string &database_name,
+                          const std::string &table_name,
+                          const std::string &old_name,
+                          const std::string &new_name,
+                          concurrency::TransactionContext *txn);
 
   //===--------------------------------------------------------------------===//
   // DEPRECATED FUNCTIONS

src/include/common/logger.h

@@ -46,7 +46,7 @@ namespace peloton {
 
 #define LOG_LOG_TIME_FORMAT "%Y-%m-%d %H:%M:%S"
 #define LOG_OUTPUT_STREAM stdout
-#define LOG_LEVEL LOG_LEVEL_TRACE
+
 // Compile Option
 #ifndef LOG_LEVEL
 // TODO : any way to use pragma message in GCC?

src/include/parser/alter_statement.h

@@ -18,11 +18,12 @@
 namespace peloton {
 namespace parser {
 /**
- * @struct AlterTableStatement
+ * @class AlterTableStatement
  * @brief Represents "ALTER TABLE add column COLUMN_NAME COLUMN_TYPE"
  * TODO: add implementation of AlterTableStatement
  */
-struct AlterTableStatement : TableRefStatement {
+class AlterTableStatement : public TableRefStatement {
+ public:
   enum class AlterTableType { INVALID = 0, ADD = 1, DROP = 2, RENAME = 3 };
   AlterTableStatement(AlterTableType type)
       : TableRefStatement(StatementType::ALTER),