addressed comments and fix varchar

Author: Dean Chen

src/catalog/catalog.cpp

@@ -947,10 +947,9 @@ std::shared_ptr<SystemCatalogs> Catalog::GetSystemCatalogs(
  * @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,
-    UNUSED_ATTRIBUTE std::unique_ptr<catalog::Schema> &new_schema,
-    UNUSED_ATTRIBUTE concurrency::TransactionContext *txn) {
+ResultType Catalog::AlterTable(oid_t database_oid, oid_t table_oid,
+                               std::unique_ptr<catalog::Schema> &new_schema,
+                               concurrency::TransactionContext *txn) {
   LOG_TRACE("AlterTable in Catalog");
 
   if (txn == nullptr)
@@ -1007,7 +1006,6 @@ ResultType Catalog::AlterTable(
             old_index->GetMetadata()->GetIndexType(),
             old_index->GetMetadata()->GetIndexConstraintType(),
             new_schema.get(),
-            // catalog::Schema::CopySchema(old_index->GetKeySchema()),
             catalog::Schema::CopySchema(new_schema.get(), new_key_attrs),
             new_key_attrs, old_index->GetMetadata()->HasUniqueKeys());
 
@@ -1060,29 +1058,26 @@ ResultType Catalog::AlterTable(
                new_column_id < new_schema->GetColumnCount(); new_column_id++) {
             auto it = column_map.find(new_column_id);
             type::Value val;
-            auto cast_flag = false;
             if (it == column_map.end()) {
               // new column, set value to null
               val = type::ValueFactory::GetNullValueByType(
                   new_schema->GetColumn(new_column_id).GetType());
             } else {
               // otherwise, copy value in old table
-              // TODO: Change type if necessary
               val = result_tile->GetValue(i, it->second);
-              if (new_schema->GetColumn(new_column_id).GetType() != old_schema->GetColumn(it->second).GetType()) {
-                //change the value's type
-                LOG_TRACE("CASTED: %s TO %s", val.GetInfo().c_str(),new_schema->GetColumn(new_column_id).GetInfo()
-                    .c_str());
-                auto casted_val = val.CastAs(new_schema->GetColumn(new_column_id).GetType());
-                cast_flag = true;
+              if (new_schema->GetColumn(new_column_id).GetType() !=
+                  old_schema->GetColumn(it->second).GetType()) {
+                // change the value's type
+                LOG_TRACE(
+                    "CASTED: %s TO %s", val.GetInfo().c_str(),
+                    new_schema->GetColumn(new_column_id).GetInfo().c_str());
+                auto casted_val =
+                    val.CastAs(new_schema->GetColumn(new_column_id).GetType());
                 tuple->SetValue(new_column_id, casted_val, pool_.get());
+              } else {
+                tuple->SetValue(new_column_id, val, pool_.get());
               }
             }
-            if (!cast_flag) {
-              tuple->SetValue(new_column_id, val, pool_.get());
-            } else {
-              LOG_TRACE("CASTED: %s", val.GetInfo().c_str());
-            }
           }
           // insert new tuple into new table
           planner::InsertPlan node(new_table, std::move(tuple));
@@ -1098,19 +1093,19 @@ ResultType Catalog::AlterTable(
       oid_t column_offset = 0;
       for (auto new_column : new_schema->GetColumns()) {
         catalog::ColumnCatalog::GetInstance()->InsertColumn(
-            new_table->GetOid(), new_column.GetName(), column_offset,
+            table_oid, new_column.GetName(), column_offset,
             new_column.GetOffset(), new_column.GetType(),
             new_column.IsInlined(), new_column.GetConstraints(), pool_.get(),
             txn);
         column_offset++;
       }
-      // Record table drop
+      // TODO: Add gc logic
       // txn->RecordDrop(database_oid, old_table->GetOid(), INVALID_OID);
 
       // Final step of physical change should be moved to commit time
       database->ReplaceTableWithOid(table_oid, new_table);
 
-      LOG_TRACE("Alter table with oid %d", new_table->GetOid());
+      LOG_TRACE("Alter table with oid %d succeed.", table_oid);
     } catch (CatalogException &e) {
       LOG_TRACE("Alter table failed.");
       return ResultType::FAILURE;

src/catalog/column.cpp

@@ -34,6 +34,7 @@ void Column::SetInlined() {
   switch (column_type) {
     case type::TypeId::VARCHAR:
     case type::TypeId::VARBINARY:
+      is_inlined = false;
       break;  // No change of inlined setting
 
     default:

src/executor/alter_executor.cpp

@@ -131,16 +131,12 @@ bool AlterExecutor::AlterTable(const peloton::planner::AlterPlan &node,
       txn->SetResult(ResultType::FAILURE);
       return false;
     } else {
-      if (change_pair.second != type::TypeId::VARCHAR) {
-        columns[i].SetType(change_pair.second);
-        columns[i].SetLength(type::VarlenType::GetTypeSize(change_pair.second));
-      } else {
-        //TODO decide VARCHAR's size when change type
-        //It is broken now!
-        columns[i].SetType(change_pair.second);
-        //columns[i].SetLength(type::VarlenType::GetTypeSize(change_pair.second));
-        columns[i].SetInlined();
-      }
+      columns[i].SetType(change_pair.second);
+      columns[i].SetInlined();
+      columns[i].SetLength(type::VarlenType::GetTypeSize(change_pair.second));
+
+      // TODO: decide VARCHAR's size when change type
+      // if (change_pair.second == type::TypeId::VARCHAR) {}
     }
   }