Refactor C++ group code and add more Vamana unit tests (#363)

Author: jparismorgan

src/include/index/index_group.h

@@ -96,9 +96,8 @@ class base_index_group {
   tiledb::Context cached_ctx_;
   std::string group_uri_;
   TemporalPolicy temporal_policy_{TimeTravel, 0};
-  size_t index_timestamp_{0};
-  size_t group_timestamp_{0};
-  size_t timetravel_index_{0};
+  size_t base_array_timestamp_{0};
+  size_t history_index_{0};
 
   // std::reference_wrapper<const index_type> index_;
   std::string version_;
@@ -151,7 +150,7 @@ class base_index_group {
    * @param ctx
    */
   void init_for_open() {
-    if (!exists(cached_ctx_)) {
+    if (!exists()) {
       throw std::runtime_error(
           "Group uri " + std::string(group_uri_) + " does not exist.");
     }
@@ -196,25 +195,22 @@ class base_index_group {
     if (size(metadata_.ingestion_timestamps_) == 0) {
       throw std::runtime_error("No ingestion timestamps found.");
     }
-    if (index_timestamp_ == 0) {
-      index_timestamp_ = metadata_.ingestion_timestamps_.back();
-      temporal_policy_ = TemporalPolicy{TimeTravel, index_timestamp_};
+    if (base_array_timestamp_ == 0) {
+      base_array_timestamp_ = metadata_.ingestion_timestamps_.back();
+      temporal_policy_ = TemporalPolicy{TimeTravel, base_array_timestamp_};
     }
 
     auto timestamp_bound = std::lower_bound(
         begin(metadata_.ingestion_timestamps_),
         end(metadata_.ingestion_timestamps_),
-        index_timestamp_);
+        base_array_timestamp_);
     if (timestamp_bound == end(metadata_.ingestion_timestamps_)) {
       // We may try to load the index at a timestamp beyond the latest ingestion
       // timestamp. In this case, use the last timestamp.
       timestamp_bound = end(metadata_.ingestion_timestamps_) - 1;
     }
-    timetravel_index_ =
+    history_index_ =
         std::distance(begin(metadata_.ingestion_timestamps_), timestamp_bound);
-
-    // @todo Or index_timestamp_?
-    group_timestamp_ = metadata_.ingestion_timestamps_[timetravel_index_];
   }
 
   /**
@@ -233,16 +229,15 @@ class base_index_group {
    * @param version
    */
   void open_for_write() {
-    if (exists(cached_ctx_)) {
+    if (exists()) {
       /** Load the current group metadata */
       init_for_open();
       if (!metadata_.ingestion_timestamps_.empty() &&
-          index_timestamp_ < metadata_.ingestion_timestamps_.back()) {
+          base_array_timestamp_ < metadata_.ingestion_timestamps_.back()) {
         throw std::runtime_error(
-            "Requested write timestamp " + std::to_string(index_timestamp_) +
-            " is not greater than " +
+            "Requested write timestamp " +
+            std::to_string(base_array_timestamp_) + " is not greater than " +
             std::to_string(metadata_.ingestion_timestamps_.back()));
-        group_timestamp_ = index_timestamp_;
       }
     } else {
       /** Create a new group */
@@ -308,7 +303,7 @@ class base_index_group {
       : cached_ctx_(ctx)
       , group_uri_(uri)
       , temporal_policy_(temporal_policy)
-      , index_timestamp_(temporal_policy.timestamp_end())
+      , base_array_timestamp_(temporal_policy.timestamp_end())
       , version_(version)
       , opened_for_(rw) {
     switch (opened_for_) {
@@ -347,8 +342,8 @@ class base_index_group {
    * @brief Test whether the group exists or not.
    * @param ctx
    */
-  bool exists(const tiledb::Context& ctx) const {
-    return tiledb::Object::object(ctx, group_uri_).type() ==
+  bool exists() const {
+    return tiledb::Object::object(cached_ctx_, group_uri_).type() ==
            tiledb::Object::Type::Group;
   }
 
@@ -362,16 +357,12 @@ class base_index_group {
     return tiledb::Object::remove(ctx, group_uri_);
   }
 
-  /**************************************************************************
-   * Getters for read and write timestamps and sizes
-   **************************************************************************/
-
   /** Temporary until time traveling is implemented */
   auto get_previous_ingestion_timestamp() const {
     return metadata_.ingestion_timestamps_.back();
   }
   auto get_ingestion_timestamp() const {
-    return metadata_.ingestion_timestamps_[timetravel_index_];
+    return metadata_.ingestion_timestamps_[history_index_];
   }
   auto append_ingestion_timestamp(size_t timestamp) {
     metadata_.ingestion_timestamps_.push_back(timestamp);
@@ -380,14 +371,11 @@ class base_index_group {
     return metadata_.ingestion_timestamps_;
   }
 
-  /*
-   * Base size information
-   */
   auto get_previous_base_size() const {
     return metadata_.base_sizes_.back();
   }
   auto get_base_size() const {
-    return metadata_.base_sizes_[timetravel_index_];
+    return metadata_.base_sizes_[history_index_];
   }
   auto append_base_size(size_t size) {
     metadata_.base_sizes_.push_back(size);
@@ -410,40 +398,34 @@ class base_index_group {
     metadata_.dimension_ = dim;
   }
 
-  auto get_timetravel_index() const {
-    return timetravel_index_;
+  auto get_history_index() const {
+    return history_index_;
   }
 
-  /**************************************************************************
-   * Getters for names and uris
-   **************************************************************************/
-
   [[nodiscard]] auto ids_uri() const {
     return array_key_to_uri("ids_array_name");
   }
   [[nodiscard]] auto ids_array_name() const {
     return array_key_to_array_name("ids_array_name");
   }
+
   [[nodiscard]] const std::reference_wrapper<const tiledb::Context> cached_ctx()
       const {
     return cached_ctx_;
   }
   [[nodiscard]] std::reference_wrapper<const tiledb::Context> cached_ctx() {
     return cached_ctx_;
   }
-  [[nodiscard]] auto group_timestamp() const {
-    return group_timestamp_;
-  }
 
   /**************************************************************************
    * Helpful functions for debugging, testing, etc
    **************************************************************************/
 
   auto set_ingestion_timestamp(size_t timestamp) {
-    metadata_.ingestion_timestamps_[timetravel_index_] = timestamp;
+    metadata_.ingestion_timestamps_[history_index_] = timestamp;
   }
   auto set_base_size(size_t size) {
-    metadata_.base_sizes_[timetravel_index_] = size;
+    metadata_.base_sizes_[history_index_] = size;
   }
 
   auto set_last_ingestion_timestamp(size_t timestamp) {
@@ -485,7 +467,7 @@ class base_index_group {
    *
    * @param msg Optional message to print before the dump.
    */
-  auto dump(const std::string& msg = "") {
+  auto dump(const std::string& msg = "") const {
     if (!empty(msg)) {
       std::cout << "-------------------------------------------------------\n";
       std::cout << "# " + msg << std::endl;
@@ -502,6 +484,10 @@ class base_index_group {
       }
       std::cout << *name << " " << member.uri() << std::endl;
     }
+    std::cout << "version_: " << version_ << std::endl;
+    std::cout << "history_index_: " << history_index_ << std::endl;
+    std::cout << "base_array_timestamp_: " << base_array_timestamp_
+              << std::endl;
     std::cout << "-------------------------------------------------------\n";
     std::cout << "# Metadata:" << std::endl;
     std::cout << "-------------------------------------------------------\n";

src/include/index/ivf_flat_group.h

@@ -109,7 +109,7 @@ class ivf_flat_index_group
     return metadata_.partition_history_.back();
   }
   auto get_num_partitions() const {
-    return metadata_.partition_history_[this->timetravel_index_];
+    return metadata_.partition_history_[this->history_index_];
   }
   auto append_num_partitions(size_t size) {
     metadata_.partition_history_.push_back(size);
@@ -119,7 +119,7 @@ class ivf_flat_index_group
   }
 
   auto set_num_partitions(size_t size) {
-    metadata_.partition_history_[this->timetravel_index_] = size;
+    metadata_.partition_history_[this->history_index_] = size;
   }
   auto set_last_num_partitions(size_t size) {
     metadata_.partition_history_.back() = size;

src/include/index/vamana_group.h

@@ -132,7 +132,7 @@ class vamana_index_group : public base_index_group<vamana_index_group<Index>> {
     return metadata_.num_edges_history_.back();
   }
   auto get_num_edges() const {
-    return metadata_.num_edges_history_[this->timetravel_index_];
+    return metadata_.num_edges_history_[this->history_index_];
   }
   auto append_num_edges(size_t size) {
     metadata_.num_edges_history_.push_back(size);
@@ -141,7 +141,7 @@ class vamana_index_group : public base_index_group<vamana_index_group<Index>> {
     return metadata_.num_edges_history_;
   }
   auto set_num_edges(size_t size) {
-    metadata_.num_edges_history_[this->timetravel_index_] = size;
+    metadata_.num_edges_history_[this->history_index_] = size;
   }
   auto set_last_num_edges(size_t size) {
     metadata_.num_edges_history_.back() = size;

src/include/test/unit_api_vamana_index.cc

@@ -588,7 +588,7 @@ TEST_CASE(
         adjacency_row_index_type_type>(ctx, index_uri);
     CHECK(typed_index.group().get_dimension() == dimensions);
     CHECK(typed_index.group().get_temp_size() == 0);
-    CHECK(typed_index.group().get_timetravel_index() == 0);
+    CHECK(typed_index.group().get_history_index() == 0);
 
     CHECK(typed_index.group().get_base_size() == 0);
     CHECK(typed_index.group().get_ingestion_timestamp() == 0);
@@ -650,7 +650,7 @@ TEST_CASE(
         adjacency_row_index_type_type>(ctx, index_uri);
     CHECK(typed_index.group().get_dimension() == dimensions);
     CHECK(typed_index.group().get_temp_size() == 0);
-    CHECK(typed_index.group().get_timetravel_index() == 0);
+    CHECK(typed_index.group().get_history_index() == 0);
 
     CHECK(typed_index.group().get_base_size() == 4);
     CHECK(typed_index.group().get_ingestion_timestamp() == 99);
@@ -714,7 +714,7 @@ TEST_CASE(
         adjacency_row_index_type_type>(ctx, index_uri);
     CHECK(typed_index.group().get_dimension() == dimensions);
     CHECK(typed_index.group().get_temp_size() == 0);
-    CHECK(typed_index.group().get_timetravel_index() == 1);
+    CHECK(typed_index.group().get_history_index() == 1);
 
     CHECK(typed_index.group().get_base_size() == 5);
     CHECK(typed_index.group().get_ingestion_timestamp() == 100);
@@ -770,7 +770,7 @@ TEST_CASE(
         adjacency_row_index_type_type>(ctx, index_uri, temporal_policy);
     CHECK(typed_index.group().get_dimension() == dimensions);
     CHECK(typed_index.group().get_temp_size() == 0);
-    CHECK(typed_index.group().get_timetravel_index() == 0);
+    CHECK(typed_index.group().get_history_index() == 0);
 
     CHECK(typed_index.group().get_base_size() == 4);
     CHECK(typed_index.group().get_ingestion_timestamp() == 99);