make changes according to pooja's reviews

mengranwo · mengranwo · commit 1599922e6547 · 2018-04-27T22:29:47.000-04:00
diff --git a/src/catalog/abstract_catalog.cpp b/src/catalog/abstract_catalog.cpp
@@ -317,6 +317,10 @@ bool AbstractCatalog::UpdateWithIndexScan(
   auto index = catalog_table_->GetIndex(index_offset);
   std::vector<oid_t> key_column_offsets =
       index->GetMetadata()->GetKeySchema()->GetIndexedColumns();
+
+  // NOTE: For indexed scan on catalog tables, we expect it not to be "partial
+  // indexed scan"(efficiency purpose).That being said, indexed column number
+  // must be equal to passed in "scan_values" size
   PELOTON_ASSERT(scan_values.size() == key_column_offsets.size());
   std::vector<ExpressionType> expr_types(scan_values.size(),
                                          ExpressionType::COMPARE_EQUAL);
diff --git a/src/catalog/catalog.cpp b/src/catalog/catalog.cpp
@@ -74,10 +74,12 @@ Catalog::Catalog() : pool_(new type::EphemeralPool()) {
   txn_manager.CommitTransaction(txn);
 }
 
-/* This function *MUST* be called after a new database is created to bootstrap
- * all system catalog tables for that database.
- * The system catalog tables must be created in certain order to make sure
- * all tuples are indexed (actually this might be fine now after Paulo's fix)
+/*@brief   This function *MUST* be called after a new database is created to
+ * bootstrap all system catalog tables for that database. The system catalog
+ * tables must be created in certain order to make sure all tuples are indexed
+ *
+ * @param   database    database which this system catalogs belong to
+ * @param   txn         transaction context
  */
 void Catalog::BootstrapSystemCatalogs(storage::Database *database,
                                       concurrency::TransactionContext *txn) {
diff --git a/src/catalog/column_catalog.cpp b/src/catalog/column_catalog.cpp
@@ -137,7 +137,7 @@ std::unique_ptr<catalog::Schema> ColumnCatalog::InitializeSchema() {
 
 bool ColumnCatalog::InsertColumn(oid_t table_oid,
                                  const std::string &column_name,
-                                 uint32_t column_id, uint32_t column_offset,
+                                 oid_t column_id, oid_t column_offset,
                                  type::TypeId column_type, bool is_inlined,
                                  const std::vector<Constraint> &constraints,
                                  type::AbstractPool *pool,
diff --git a/src/catalog/system_catalogs.cpp b/src/catalog/system_catalogs.cpp
@@ -21,18 +21,23 @@
 namespace peloton {
 namespace catalog {
 
+/*@brief    system catalog constructor, create core catalog tables and manually
+ * insert records into pg_attribute
+ * @param   database    the database which the catalog tables belongs to
+ * @param   txn         TransactionContext
+ */
 SystemCatalogs::SystemCatalogs(storage::Database *database,
                                type::AbstractPool *pool,
                                concurrency::TransactionContext *txn)
-    : pg_trigger(nullptr),
-      pg_table_metrics(nullptr),
-      pg_index_metrics(nullptr),
-      pg_query_metrics(nullptr) {
+    : pg_trigger_(nullptr),
+      pg_table_metrics_(nullptr),
+      pg_index_metrics_(nullptr),
+      pg_query_metrics_(nullptr) {
   oid_t database_oid = database->GetOid();
-  pg_attribute = new ColumnCatalog(database, pool, txn);
-  pg_namespace = new SchemaCatalog(database, pool, txn);
-  pg_table = new TableCatalog(database, pool, txn);
-  pg_index = new IndexCatalog(database, pool, txn);
+  pg_attribute_ = new ColumnCatalog(database, pool, txn);
+  pg_namespace_ = new SchemaCatalog(database, pool, txn);
+  pg_table_ = new TableCatalog(database, pool, txn);
+  pg_index_ = new IndexCatalog(database, pool, txn);
 
   // TODO: can we move this to BootstrapSystemCatalogs()?
   // insert column information into pg_attribute
@@ -49,52 +54,53 @@ SystemCatalogs::SystemCatalogs(storage::Database *database,
              ->GetTableWithOid(shared_tables[i].first, shared_tables[i].second)
              ->GetSchema()
              ->GetColumns()) {
-      pg_attribute->InsertColumn(shared_tables[i].second, column.GetName(),
-                                 column_id, column.GetOffset(),
-                                 column.GetType(), column.IsInlined(),
-                                 column.GetConstraints(), pool, txn);
+      pg_attribute_->InsertColumn(shared_tables[i].second, column.GetName(),
+                                  column_id, column.GetOffset(),
+                                  column.GetType(), column.IsInlined(),
+                                  column.GetConstraints(), pool, txn);
       column_id++;
     }
   }
 }
 
 SystemCatalogs::~SystemCatalogs() {
-  delete pg_index;
-  delete pg_table;
-  delete pg_attribute;
-  delete pg_namespace;
-  if (pg_trigger) delete pg_trigger;
+  delete pg_index_;
+  delete pg_table_;
+  delete pg_attribute_;
+  delete pg_namespace_;
+  if (pg_trigger_) delete pg_trigger_;
   // if (pg_proc) delete pg_proc;
-  if (pg_table_metrics) delete pg_table_metrics;
-  if (pg_index_metrics) delete pg_index_metrics;
-  if (pg_query_metrics) delete pg_query_metrics;
+  if (pg_table_metrics_) delete pg_table_metrics_;
+  if (pg_index_metrics_) delete pg_index_metrics_;
+  if (pg_query_metrics_) delete pg_query_metrics_;
 }
 
-/*
- * @brief   using sql create statement to create secondary catalog tables
+/*@brief    using sql create statement to create secondary catalog tables
+ * @param   database_name    the database which the namespace belongs to
+ * @param   txn              TransactionContext
  */
 void SystemCatalogs::Bootstrap(const std::string &database_name,
                                concurrency::TransactionContext *txn) {
   LOG_DEBUG("Bootstrapping database: %s", database_name.c_str());
 
-  if (!pg_trigger) {
-    pg_trigger = new TriggerCatalog(database_name, txn);
+  if (!pg_trigger_) {
+    pg_trigger_ = new TriggerCatalog(database_name, txn);
   }
 
   // if (!pg_proc) {
   //     pg_proc = new ProcCatalog(database_name, txn);
   // }
 
-  if (!pg_table_metrics) {
-    pg_table_metrics = new TableMetricsCatalog(database_name, txn);
+  if (!pg_table_metrics_) {
+    pg_table_metrics_ = new TableMetricsCatalog(database_name, txn);
   }
 
-  if (!pg_index_metrics) {
-    pg_index_metrics = new IndexMetricsCatalog(database_name, txn);
+  if (!pg_index_metrics_) {
+    pg_index_metrics_ = new IndexMetricsCatalog(database_name, txn);
   }
 
-  if (!pg_query_metrics) {
-    pg_query_metrics = new QueryMetricsCatalog(database_name, txn);
+  if (!pg_query_metrics_) {
+    pg_query_metrics_ = new QueryMetricsCatalog(database_name, txn);
   }
 }
 
diff --git a/src/catalog/table_catalog.cpp b/src/catalog/table_catalog.cpp
@@ -410,7 +410,7 @@ bool TableCatalog::InsertTable(oid_t table_oid, const std::string &table_name,
 /*@brief   delete a tuple about table info from pg_table(using index scan)
  * @param   table_oid
  * @param   txn     TransactionContext
- * @return  Whether deletion is Successful
+ * @return  Whether deletion is successful
  */
 bool TableCatalog::DeleteTable(oid_t table_oid,
                                concurrency::TransactionContext *txn) {
@@ -561,6 +561,12 @@ TableCatalog::GetTableObjects(concurrency::TransactionContext *txn) {
   return database_object->GetTableObjects();
 }
 
+/*@brief    update version id column within pg_table
+ * @param   update_val   the new(updated) version id
+ * @param   table_oid    which table to be updated
+ * @param   txn          TransactionContext
+ * @return  Whether update is successful
+ */
 bool TableCatalog::UpdateVersionId(oid_t update_val, oid_t table_oid,
                                    concurrency::TransactionContext *txn) {
   std::vector<oid_t> update_columns({ColumnId::VERSION_ID});  // version_id
diff --git a/src/include/catalog/catalog_defaults.h b/src/include/catalog/catalog_defaults.h
@@ -35,6 +35,7 @@ namespace catalog {
 
 // Local oids from START_OID = 0 to START_OID + OID_OFFSET are reserved
 #define OID_OFFSET 100
+#define CATALOG_TABLES_COUNT 8
 
 // Oid mask for each type
 #define DATABASE_OID_MASK (static_cast<oid_t>(catalog::CatalogType::DATABASE))
diff --git a/src/include/catalog/column_catalog.h b/src/include/catalog/column_catalog.h
@@ -81,7 +81,7 @@ class ColumnCatalog : public AbstractCatalog {
   // write Related API
   //===--------------------------------------------------------------------===//
   bool InsertColumn(oid_t table_oid, const std::string &column_name,
-                    uint32_t column_id, uint32_t column_offset,
+                    oid_t column_id, oid_t column_offset,
                     type::TypeId column_type, bool is_inlined,
                     const std::vector<Constraint> &constraints,
                     type::AbstractPool *pool,
diff --git a/src/include/catalog/query_metrics_catalog.h b/src/include/catalog/query_metrics_catalog.h
@@ -70,7 +70,7 @@ class QueryMetricsCatalog : public AbstractCatalog {
   int64_t GetNumParams(const std::string &name,
                        concurrency::TransactionContext *txn);
   // TODO: In theory, we don't need database_oid
-  // but now we store all the query metrics under default database "pelton"
+  // but now we store all the query metrics under default database "peloton"
   enum ColumnId {
     NAME = 0,
     DATABASE_OID = 1,
diff --git a/src/include/catalog/system_catalogs.h b/src/include/catalog/system_catalogs.h
@@ -47,73 +47,77 @@ class SystemCatalogs {
   void Bootstrap(const std::string &database_name,
                  concurrency::TransactionContext *txn);
 
+  //===--------------------------------------------------------------------===//
+  // GET FUNCTIONS
+  // get catalog tables with name
+  //===--------------------------------------------------------------------===//
   ColumnCatalog *GetColumnCatalog() {
-    if (!pg_attribute) {
+    if (!pg_attribute_) {
       throw CatalogException("Column catalog has not been initialized");
     }
-    return pg_attribute;
+    return pg_attribute_;
   }
 
   SchemaCatalog *GetSchemaCatalog() {
-    if (!pg_namespace) {
+    if (!pg_namespace_) {
       throw CatalogException("schema catalog has not been initialized");
     }
-    return pg_namespace;
+    return pg_namespace_;
   }
 
   TableCatalog *GetTableCatalog() {
-    if (!pg_table) {
+    if (!pg_table_) {
       throw CatalogException("Table catalog has not been initialized");
     }
-    return pg_table;
+    return pg_table_;
   }
 
   IndexCatalog *GetIndexCatalog() {
-    if (!pg_index) {
+    if (!pg_index_) {
       throw CatalogException("Index catalog has not been initialized");
     }
-    return pg_index;
+    return pg_index_;
   }
 
   TriggerCatalog *GetTriggerCatalog() {
-    if (!pg_trigger) {
+    if (!pg_trigger_) {
       throw CatalogException("Trigger catalog has not been initialized");
     }
-    return pg_trigger;
+    return pg_trigger_;
   }
 
   TableMetricsCatalog *GetTableMetricsCatalog() {
-    if (!pg_table_metrics) {
+    if (!pg_table_metrics_) {
       throw CatalogException("Table metrics catalog has not been initialized");
     }
-    return pg_table_metrics;
+    return pg_table_metrics_;
   }
 
   IndexMetricsCatalog *GetIndexMetricsCatalog() {
-    if (!pg_index_metrics) {
+    if (!pg_index_metrics_) {
       throw CatalogException("Index metrics catalog has not been initialized");
     }
-    return pg_index_metrics;
+    return pg_index_metrics_;
   }
 
   QueryMetricsCatalog *GetQueryMetricsCatalog() {
-    if (!pg_query_metrics) {
+    if (!pg_query_metrics_) {
       throw CatalogException("Query metrics catalog has not been initialized");
     }
-    return pg_query_metrics;
+    return pg_query_metrics_;
   }
 
  private:
-  ColumnCatalog *pg_attribute;
-  SchemaCatalog *pg_namespace;
-  TableCatalog *pg_table;
-  IndexCatalog *pg_index;
+  ColumnCatalog *pg_attribute_;
+  SchemaCatalog *pg_namespace_;
+  TableCatalog *pg_table_;
+  IndexCatalog *pg_index_;
 
-  TriggerCatalog *pg_trigger;
+  TriggerCatalog *pg_trigger_;
   // ProcCatalog *pg_proc;
-  TableMetricsCatalog *pg_table_metrics;
-  IndexMetricsCatalog *pg_index_metrics;
-  QueryMetricsCatalog *pg_query_metrics;
+  TableMetricsCatalog *pg_table_metrics_;
+  IndexMetricsCatalog *pg_index_metrics_;
+  QueryMetricsCatalog *pg_query_metrics_;
 };
 
 }  // namespace catalog
diff --git a/src/include/planner/plan_util.h b/src/include/planner/plan_util.h
@@ -55,6 +55,7 @@ class PlanUtil {
 
   /**
    * @brief Get the indexes affected by a given query
+   * @param CatalogCache
    * @param SQLStatement
    * @return set of affected index object ids
    */
diff --git a/src/include/planner/seq_scan_plan.h b/src/include/planner/seq_scan_plan.h
@@ -17,10 +17,10 @@
 #include <vector>
 
 #include "abstract_scan_plan.h"
-#include "common/internal_types.h"
 #include "common/logger.h"
 #include "expression/abstract_expression.h"
 #include "type/serializer.h"
+#include "common/internal_types.h"
 
 namespace peloton {
 
diff --git a/src/parser/postgresparser.cpp b/src/parser/postgresparser.cpp
@@ -1388,7 +1388,8 @@ parser::DropStatement *PostgresParser::DropTableTransform(DropStmt *root) {
     auto table_info = new TableInfo{};
     auto table_list = reinterpret_cast<List *>(cell->data.ptr_value);
     LOG_TRACE("%d", ((Node *)(table_list->head->data.ptr_value))->type);
-    // if schema name is specified
+    // if schema name is specified, which means you are using the syntax like
+    // DROP INDEX/TABLE A.B where A is schema name and B is table/index name
     if (table_list->length == 2) {
       table_info->schema_name =
           reinterpret_cast<value *>(table_list->head->data.ptr_value)->val.str;
@@ -1412,8 +1413,10 @@ parser::DropStatement *PostgresParser::DropTriggerTransform(DropStmt *root) {
   // first, set trigger name
   result->SetTriggerName(
       reinterpret_cast<value *>(list->tail->data.ptr_value)->val.str);
-  // if schema name is specified
   TableInfo *table_info = new TableInfo{};
+  // if schema name is specified, which means you are using the syntax like
+  // DROP TRIGGER A.B.C where A is schema name, B is table name and C is trigger
+  // name
   if (list->length == 3) {
     table_info->schema_name =
         reinterpret_cast<value *>(list->head->data.ptr_value)->val.str;
@@ -1448,7 +1451,8 @@ parser::DropStatement *PostgresParser::DropIndexTransform(DropStmt *root) {
   auto result = new DropStatement(DropStatement::EntityType::kIndex);
   auto cell = root->objects->head;
   auto list = reinterpret_cast<List *>(cell->data.ptr_value);
-  // if schema name is specified
+  // if schema name is specified, which means you are using the syntax like
+  // DROP INDEX/TABLE A.B where A is schema name and B is table/index name
   if (list->length == 2) {
     TableInfo *table_info = new TableInfo{};
     table_info->schema_name =
diff --git a/src/statistics/stats_aggregator.cpp b/src/statistics/stats_aggregator.cpp
@@ -161,13 +161,6 @@ void StatsAggregator::UpdateQueryMetrics(int64_t time_stamp,
     }
 
     // Generate and insert the tuple
-    // auto query_tuple = catalog::GetQueryMetricsCatalogTuple(
-    //     query_metrics_table->GetSchema(), query_metric->GetName(),
-    //     query_metric->GetDatabaseId(), num_params, type_buf, format_buf,
-    //     value_buf, reads, updates, deletes, inserts, (int64_t)latency,
-    //     (int64_t)(cpu_system + cpu_user), time_stamp, pool_.get());
-    // catalog::InsertTuple(query_metrics_table, std::move(query_tuple), txn);
-
     catalog::Catalog::GetInstance()
         ->GetSystemCatalogs(query_metric->GetDatabaseId())
         ->GetQueryMetricsCatalog()
diff --git a/test/binder/binder_test.cpp b/test/binder/binder_test.cpp
@@ -42,7 +42,6 @@ class BinderCorrectnessTest : public PelotonTest {
     PelotonTest::SetUp();
     catalog::Catalog::GetInstance();
     // NOTE: Catalog::GetInstance()->Bootstrap(), you can only call it once!
-    // catalog->Bootstrap();
     TestingExecutorUtil::InitializeDatabase(DEFAULT_DB_NAME);
   }
 
diff --git a/test/executor/drop_test.cpp b/test/executor/drop_test.cpp
@@ -77,7 +77,6 @@ TEST_F(DropTests, DroppingTable) {
   auto catalog = catalog::Catalog::GetInstance();
   // NOTE: Catalog::GetInstance()->Bootstrap() has been called in previous tests
   // you can only call it once!
-  // catalog->Bootstrap();
 
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
   auto txn = txn_manager.BeginTransaction();
diff --git a/test/function/functions_test.cpp b/test/function/functions_test.cpp
@@ -31,16 +31,14 @@ class FunctionsTests : public PelotonTest {
 
   virtual void SetUp() {
     PelotonTest::SetUp();
-    // NOTE: Catalog::GetInstance()->Bootstrap() has been called in previous
-    // unit tests you can only call it once!
-    // auto catalog =
-    // catalog::Catalog::GetInstance(); catalog->Bootstrap();
+    // Bootstrap catalog
+    auto catalog = catalog::Catalog::GetInstance();
+    catalog->Bootstrap();
   }
 };
 
 TEST_F(FunctionsTests, CatalogTest) {
   auto catalog = catalog::Catalog::GetInstance();
-  catalog->Bootstrap();
 
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
   auto &pg_language = catalog::LanguageCatalog::GetInstance();
diff --git a/test/optimizer/optimizer_test.cpp b/test/optimizer/optimizer_test.cpp
diff --git a/test/optimizer/tuple_samples_storage_test.cpp b/test/optimizer/tuple_samples_storage_test.cpp
diff --git a/test/settings/settings_manager_test.cpp b/test/settings/settings_manager_test.cpp
diff --git a/test/sql/timestamp_functions_sql_test.cpp b/test/sql/timestamp_functions_sql_test.cpp
diff --git a/test/sql/type_sql_test.cpp b/test/sql/type_sql_test.cpp
diff --git a/test/storage/database_test.cpp b/test/storage/database_test.cpp
diff --git a/test/trigger/trigger_test.cpp b/test/trigger/trigger_test.cpp
diff --git a/test/udf/udf_test.cpp b/test/udf/udf_test.cpp
