Fixing comments for CatalogCache methods

apavlo · apavlo · commit 2d3fab887e32 · 2018-06-19T07:59:31.000-04:00
diff --git a/src/catalog/catalog_cache.cpp b/src/catalog/catalog_cache.cpp
@@ -86,10 +86,6 @@ bool CatalogCache::EvictDatabaseObject(const std::string &database_name) {
   return true;
 }
 
-/*@brief   get database catalog object from cache
- * @param   database_oid
- * @return  database catalog object; if not found return object with invalid oid
- */
 std::shared_ptr<DatabaseCatalogObject> CatalogCache::GetDatabaseObject(
     oid_t database_oid) {
   auto it = database_objects_cache.find(database_oid);
@@ -99,10 +95,6 @@ std::shared_ptr<DatabaseCatalogObject> CatalogCache::GetDatabaseObject(
   return it->second;
 }
 
-/*@brief   get database catalog object from cache
- * @param   database_name
- * @return  database catalog object; if not found return null
- */
 std::shared_ptr<DatabaseCatalogObject> CatalogCache::GetDatabaseObject(
     const std::string &database_name) {
   auto it = database_name_cache.find(database_name);
@@ -120,10 +112,6 @@ std::vector<std::shared_ptr<DatabaseCatalogObject>> CatalogCache::GetAllDatabase
   return (databases);
 }
 
-/*@brief   search table catalog object from all cached database objects
- * @param   table_oid
- * @return  table catalog object; if not found return null
- */
 std::shared_ptr<TableCatalogObject> CatalogCache::GetCachedTableObject(
 		oid_t database_oid, oid_t table_oid) {
 	auto database_object = GetDatabaseObject(database_oid);
@@ -133,10 +121,6 @@ std::shared_ptr<TableCatalogObject> CatalogCache::GetCachedTableObject(
   return nullptr;
 }
 
-/*@brief   search index catalog object from all cached database objects
- * @param   index_oid
- * @return  index catalog object; if not found return null
- */
 std::shared_ptr<IndexCatalogObject> CatalogCache::GetCachedIndexObject(
 		oid_t database_oid, oid_t index_oid) {
 	auto database_object = GetDatabaseObject(database_oid);
@@ -146,10 +130,6 @@ std::shared_ptr<IndexCatalogObject> CatalogCache::GetCachedIndexObject(
   return nullptr;
 }
 
-/*@brief   search index catalog object from all cached database objects
- * @param   index_name
- * @return  index catalog object; if not found return null
- */
 std::shared_ptr<IndexCatalogObject> CatalogCache::GetCachedIndexObject(
 		const std::string &database_name, const std::string &index_name,
 		const std::string &schema_name) {
@@ -161,43 +141,32 @@ std::shared_ptr<IndexCatalogObject> CatalogCache::GetCachedIndexObject(
   return nullptr;
 }
 
-/*@brief   insert sequence catalog object into cache
- * @param  sequence_object
- * @return  false only if sequence already exists in cache or invalid
- */
 bool CatalogCache::InsertSequenceObject(
     std::shared_ptr<SequenceCatalogObject> sequence_object) {
-  if (!sequence_object || sequence_object->seq_oid == INVALID_OID) {
+  if (!sequence_object || sequence_object->GetSequenceOid() == INVALID_OID) {
     return false;  // invalid object
   }
 
-  std::size_t hash_key = GetHashKey(sequence_object->seq_name,
-                                    sequence_object->db_oid);
+  std::pair key = std::make_pair(sequence_object->GetDatabaseOid(),
+                                 sequence_object->GetName());
 
   // check if already in cache
-  if (sequence_objects_cache.find(hash_key) !=
+  if (sequence_objects_cache.find(key) !=
           sequence_objects_cache.end()) {
     LOG_DEBUG("Sequence %s already exists in cache!",
-              sequence_object->seq_name.c_str());
+              sequence_object->GetName().c_str());
     return false;
   }
 
   sequence_objects_cache.insert(
-          std::make_pair(hash_key, sequence_object));
+          std::make_pair(key, sequence_object));
   return true;
 }
 
-/*@brief   evict sequence catalog object from cache
- * @param  sequence_name
- * @param  database_oid
- * @return  true if specified sequence is found and evicted;
- *          false if not found
- */
 bool CatalogCache::EvictSequenceObject(const std::string & sequence_name,
          oid_t database_oid) {
-  std::size_t hash_key = GetHashKey(sequence_name, database_oid);
-
-  auto it = sequence_objects_cache.find(hash_key);
+  std::pair key = std::make_pair(database_oid, sequence_name);
+  auto it = sequence_objects_cache.find(key);
   if (it == sequence_objects_cache.end()) {
     return false;  // sequence not found in cache
   }
@@ -208,32 +177,16 @@ bool CatalogCache::EvictSequenceObject(const std::string & sequence_name,
   return true;
 }
 
-/*@brief   get sequence catalog object from cache
- * @param  sequence_name
- * @param  database_oid
- * @return  sequence catalog object; if not found return object with invalid oid
- */
 std::shared_ptr<SequenceCatalogObject> CatalogCache::GetSequenceObject(
             const std::string & sequence_name, oid_t database_oid) {
-  std::size_t hash_key = GetHashKey(sequence_name, database_oid);
-  auto it = sequence_objects_cache.find(hash_key);
+  std::pair key = std::make_pair(database_oid, sequence_name);
+  auto it = sequence_objects_cache.find(key);
   if (it == sequence_objects_cache.end()) {
     return nullptr;
   }
   return it->second;
 }
 
-/*@brief  get the hash key given the sequence information
- * @param  sequence_name
- * @param  database_oid
- * @return  hash key
- */
-std::size_t CatalogCache::GetHashKey(const std::string sequence_name,
-     oid_t database_oid) {
-  std::tuple<std::string, size_t> key(sequence_name, database_oid);
-  boost::hash<std::tuple<std::string, size_t>> key_hash;
-  return key_hash(key);
-}
 
 }  // namespace catalog
 }  // namespace peloton
diff --git a/src/catalog/sequence_catalog.cpp b/src/catalog/sequence_catalog.cpp
@@ -30,11 +30,6 @@
 namespace peloton {
 namespace catalog {
 
-/* @brief   Get the nextval of the sequence
- * @return  the next value of the sequence.
- * @exception throws SequenceException if the sequence exceeds the upper/lower
- * limit.
- */
 int64_t SequenceCatalogObject::GetNextVal() {
   int64_t result = seq_curr_val;
   seq_prev_val = result;
@@ -93,20 +88,7 @@ SequenceCatalog::SequenceCatalog(const std::string &database_name,
 
 SequenceCatalog::~SequenceCatalog() {}
 
-/* @brief   Insert the sequence by name.
- * @param   database_oid  the databse_oid associated with the sequence
- * @param   sequence_name the name of the sequence
- * @param   seq_increment the increment per step of the sequence
- * @param   seq_max       the max value of the sequence
- * @param   seq_min       the min value of the sequence
- * @param   seq_start     the start of the sequence
- * @param   seq_cycle     whether the sequence cycles
- * @param   pool          an instance of abstract pool
- * @param   txn           current transaction
- * @return  ResultType::SUCCESS if the sequence exists, ResultType::FAILURE
- * otherwise.
- * @exception throws SequenceException if the sequence already exists.
- */
+
 bool SequenceCatalog::InsertSequence(oid_t database_oid,
                                      std::string sequence_name,
                                      int64_t seq_increment, int64_t seq_max,
@@ -151,13 +133,6 @@ bool SequenceCatalog::InsertSequence(oid_t database_oid,
   return InsertTuple(std::move(tuple), txn);
 }
 
-/* @brief   Delete the sequence by name.
- * @param   database_name  the database name associated with the sequence
- * @param   sequence_name the name of the sequence
- * @param   txn           current transaction
- * @return  ResultType::SUCCESS if the sequence exists, throw exception
- * otherwise.
- */
 ResultType SequenceCatalog::DropSequence(const std::string &database_name,
                                          const std::string &sequence_name,
                                          concurrency::TransactionContext *txn) {
@@ -188,12 +163,6 @@ ResultType SequenceCatalog::DropSequence(const std::string &database_name,
   return ResultType::SUCCESS;
 }
 
-/* @brief   Delete the sequence by name. The sequence is guaranteed to exist.
- * @param   database_oid  the databse_oid associated with the sequence
- * @param   sequence_name the name of the sequence
- * @param   txn           current transaction
- * @return  The result of DeleteWithIndexScan.
- */
 bool SequenceCatalog::DeleteSequenceByName(
     const std::string &sequence_name, oid_t database_oid,
     concurrency::TransactionContext *txn) {
@@ -205,12 +174,6 @@ bool SequenceCatalog::DeleteSequenceByName(
   return DeleteWithIndexScan(index_offset, values, txn);
 }
 
-/* @brief   get sequence from pg_sequence table
- * @param   database_oid  the databse_oid associated with the sequence
- * @param   sequence_name the name of the sequence
- * @param   txn           current transaction
- * @return  a SequenceCatalogObject if the sequence is found, nullptr otherwise
- */
 std::shared_ptr<SequenceCatalogObject> SequenceCatalog::GetSequence(
     oid_t database_oid, const std::string &sequence_name,
     concurrency::TransactionContext *txn) {
@@ -254,12 +217,6 @@ std::shared_ptr<SequenceCatalogObject> SequenceCatalog::GetSequence(
   return new_sequence;
 }
 
-/* @brief   update the next value of the sequence in the underlying storage
- * @param   sequence_oid  the sequence_oid of the sequence
- * @param   nextval       the nextval of the sequence
- * @param   txn           current transaction
- * @return  the result of the transaction
- */
 bool SequenceCatalog::UpdateNextVal(oid_t sequence_oid, int64_t nextval,
     concurrency::TransactionContext *txn){
   std::vector<oid_t> update_columns({SequenceCatalog::ColumnId::SEQUENCE_VALUE});
@@ -272,14 +229,6 @@ bool SequenceCatalog::UpdateNextVal(oid_t sequence_oid, int64_t nextval,
   return UpdateWithIndexScan(update_columns, update_values, scan_values, index_offset, txn);
 }
 
-/* @brief   get sequence oid from pg_sequence table given sequence_name and
- * database_oid
- * @param   database_oid  the databse_oid associated with the sequence
- * @param   sequence_name the name of the sequence
- * @param   txn           current transaction
- * @return  the oid_t of the sequence if the sequence is found, INVALID_OID
- * otherwise
- */
 oid_t SequenceCatalog::GetSequenceOid(std::string sequence_name,
                                       oid_t database_oid,
                                       concurrency::TransactionContext *txn) {
diff --git a/src/function/sequence_functions.cpp b/src/function/sequence_functions.cpp
@@ -42,23 +42,24 @@ uint32_t SequenceFunctions::Nextval(executor::ExecutorContext &ctx,
   auto database_catalog = all_databases[0];
   LOG_DEBUG("Get database oid: %u", database_catalog->GetDatabaseOid());
 
+  auto sequence_catalog = catalog::Catalog::GetInstance()
+          ->GetSystemCatalogs(database_catalog->GetDatabaseOid())
+          ->GetSequenceCatalog();
+
   // initialize a new transaction for incrementing sequence value
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
   auto mini_txn = txn_manager.BeginTransaction();
 
   // evict the old cached copy of sequence
-  txn->GetCatalogCache()->EvictSequenceObject(sequence_name,
-                                              database_catalog->GetDatabaseOid());
-  auto sequence_object =
-          catalog::Catalog::GetInstance()
-          ->GetSystemCatalogs(database_catalog->GetDatabaseOid())
-          ->GetSequenceCatalog()
-          ->GetSequence(database_catalog->GetDatabaseOid(),
-                        sequence_name, mini_txn);
+//  txn->GetCatalogCache()->EvictSequenceObject(sequence_name,
+//                                              database_catalog->GetDatabaseOid());
+  auto sequence_object = sequence_catalog
+          ->GetSequence(database_catalog->GetDatabaseOid(), sequence_name, mini_txn);
 
   if (sequence_object != nullptr) {
-    uint32_t val = sequence_object->GetNextVal();
+    int64_t val = sequence_object->GetNextVal();
     int64_t curr_val = sequence_object->GetCurrVal();
+
     // insert the new copy of sequence into cache for future currval
     txn->GetCatalogCache()->InsertSequenceObject(sequence_object);
 
diff --git a/src/include/catalog/catalog_cache.h b/src/include/catalog/catalog_cache.h
@@ -52,7 +52,19 @@ class CatalogCache {
   CatalogCache &operator=(CatalogCache const &) = delete;
 
  private:
+
+  /**
+   * @brief   get database catalog object from cache
+   * @param   database_oid
+   * @return  database catalog object; if not found return object with invalid oid
+   */
   std::shared_ptr<DatabaseCatalogObject> GetDatabaseObject(oid_t database_oid);
+
+  /**
+   * @brief   get database catalog object from cache
+   * @param   database_name
+   * @return  database catalog object; if not found return null
+   */
   std::shared_ptr<DatabaseCatalogObject> GetDatabaseObject(
       const std::string &name);
 
@@ -62,8 +74,25 @@ class CatalogCache {
    */
   std::vector<std::shared_ptr<DatabaseCatalogObject>> GetAllDatabaseObjects();
 
+  /**
+   * @brief   search table catalog object from all cached database objects
+   * @param   table_oid
+   * @return  table catalog object; if not found return null
+   */
   std::shared_ptr<TableCatalogObject> GetCachedTableObject(oid_t database_oid, oid_t table_oid);
+
+  /**
+   * @brief   search index catalog object from all cached database objects
+   * @param   index_oid
+   * @return  index catalog object; if not found return null
+   */
   std::shared_ptr<IndexCatalogObject> GetCachedIndexObject(oid_t database_oid, oid_t index_oid);
+
+  /**
+   * @brief   search index catalog object from all cached database objects
+   * @param   index_name
+   * @return  index catalog object; if not found return null
+   */
   std::shared_ptr<IndexCatalogObject> GetCachedIndexObject(
       const std::string &database_name, const std::string &index_name,
       const std::string &schema_name);
@@ -74,14 +103,32 @@ class CatalogCache {
   bool EvictDatabaseObject(oid_t database_oid);
   bool EvictDatabaseObject(const std::string &database_name);
 
-  // sequence catalog cache interface
+  /**
+   * @brief   insert sequence catalog object into cache
+   * @param  sequence_object
+   * @return  false only if sequence already exists in cache or invalid
+   */
   bool InsertSequenceObject(
           std::shared_ptr<SequenceCatalogObject> sequence_object);
+
+  /**
+   * @brief   evict sequence catalog object from cache
+   * @param  sequence_name
+   * @param  database_oid
+   * @return  true if specified sequence is found and evicted;
+   *          false if not found
+   */
   bool EvictSequenceObject(const std::string &sequence_name,
           oid_t database_oid);
+
+  /**
+   * @brief   get sequence catalog object from cache
+   * @param  sequence_name
+   * @param  database_oid
+   * @return  sequence catalog object; if not found return object with invalid oid
+   */
   std::shared_ptr<SequenceCatalogObject> GetSequenceObject(
           const std::string &sequence_name, oid_t database_oid);
-  std::size_t GetHashKey(std::string sequence_name, oid_t database_oid);
 
   // cache for database catalog object
   std::unordered_map<oid_t, std::shared_ptr<DatabaseCatalogObject>>
@@ -90,7 +137,8 @@ class CatalogCache {
       database_name_cache;
 
   // cache for sequence catalog object
-  std::unordered_map<std::size_t, std::shared_ptr<SequenceCatalogObject>>
+  std::unordered_map<std::pair<oid_t, std::string>,
+                     std::shared_ptr<SequenceCatalogObject>>
       sequence_objects_cache;
 
 };
diff --git a/src/include/catalog/sequence_catalog.h b/src/include/catalog/sequence_catalog.h
diff --git a/test/function/sequence_functions_test.cpp b/test/function/sequence_functions_test.cpp
