Remove serialization non C41 constructors from stats (#4706)

With this change in, PR #4685 and others to follow should: a) remove `Subarray::set_stats` and respective call from `serialization/query.cc` b) where `Subarray is constructed in `serialization/query.cc`, change constructor call with the following code snippet: ``` auto &stats_data = stats_from_capnp(reader.getStats()); Subarray subarray(array, layout, query_stats, stats_data, dummy_logger, true); ``` c) The constructor calls parent_stats->create_child(prefix, stats_data); d) When all migrations are done, make `Stats::populate_with_data private` --- TYPE: NO_HISTORY DESC: Remove serialization non C41 constructors from stats --------- Co-authored-by: KiterLuc <[email protected]>
TileDB-Inc · Feb 12, 2024 · 9b28f75 · 9b28f75
1 parent c45956c
commit 9b28f75
Show file tree

Hide file tree

Showing 12 changed files with 277 additions and 111 deletions.
diff --git a/tiledb/sm/query/query.cc b/tiledb/sm/query/query.cc
@@ -827,6 +827,7 @@ Status Query::process() {
 
     if (!only_dim_label_query()) {
       if (strategy_ != nullptr) {
+        // The strategy destructor should reset its own Stats object here
         dynamic_cast<StrategyBase*>(strategy_.get())->stats()->reset();
         strategy_ = nullptr;
       }
@@ -917,6 +918,7 @@ Status Query::reset_strategy_with_layout(
     Layout layout, bool force_legacy_reader) {
   force_legacy_reader_ = force_legacy_reader;
   if (strategy_ != nullptr) {
+    // The strategy destructor should reset its own Stats object here
     dynamic_cast<StrategyBase*>(strategy_.get())->stats()->reset();
     strategy_ = nullptr;
   }
@@ -1663,6 +1665,10 @@ stats::Stats* Query::stats() const {
   return stats_;
 }
 
+void Query::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 shared_ptr<Buffer> Query::rest_scratch() const {
   return rest_scratch_;
 }

diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h
@@ -655,6 +655,14 @@ class Query {
   /** Returns the internal stats object. */
   stats::Stats* stats() const;
 
+  /**
+   * Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
+   */
+  void set_stats(const stats::StatsData& data);
+
   /** Returns the scratch space used for REST requests. */
   shared_ptr<Buffer> rest_scratch() const;
 

diff --git a/tiledb/sm/query/strategy_base.cc b/tiledb/sm/query/strategy_base.cc
@@ -65,6 +65,10 @@ stats::Stats* StrategyBase::stats() const {
   return stats_;
 }
 
+void StrategyBase::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 /* ****************************** */
 /*       PROTECTED METHODS        */
 /* ****************************** */

diff --git a/tiledb/sm/query/strategy_base.h b/tiledb/sm/query/strategy_base.h
@@ -208,6 +208,14 @@ class StrategyBase {
   /** Returns `stats_`. */
   stats::Stats* stats() const;
 
+  /**
+   * Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
+   */
+  void set_stats(const stats::StatsData& data);
+
   /** Returns the configured offsets format mode. */
   std::string offsets_mode() const;
 

diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc
@@ -88,7 +88,7 @@ namespace serialization {
 
 shared_ptr<Logger> dummy_logger = make_shared<Logger>(HERE(), "");
 
-Status stats_to_capnp(Stats& stats, capnp::Stats::Builder* stats_builder) {
+void stats_to_capnp(const Stats& stats, capnp::Stats::Builder* stats_builder) {
   // Build counters
   const auto counters = stats.counters();
   if (counters != nullptr && !counters->empty()) {
@@ -114,31 +114,29 @@ Status stats_to_capnp(Stats& stats, capnp::Stats::Builder* stats_builder) {
       ++index;
     }
   }
-
-  return Status::Ok();
 }
 
-Status stats_from_capnp(
-    const capnp::Stats::Reader& stats_reader, Stats* stats) {
+StatsData stats_from_capnp(const capnp::Stats::Reader& stats_reader) {
+  std::unordered_map<std::string, uint64_t> counters;
+  std::unordered_map<std::string, double> timers;
+
   if (stats_reader.hasCounters()) {
-    auto counters = stats->counters();
     auto counters_reader = stats_reader.getCounters();
     for (const auto entry : counters_reader.getEntries()) {
       auto key = std::string_view{entry.getKey().cStr(), entry.getKey().size()};
-      (*counters)[std::string{key}] = entry.getValue();
+      counters[std::string(key)] = entry.getValue();
     }
   }
 
   if (stats_reader.hasTimers()) {
-    auto timers = stats->timers();
     auto timers_reader = stats_reader.getTimers();
     for (const auto entry : timers_reader.getEntries()) {
       auto key = std::string_view{entry.getKey().cStr(), entry.getKey().size()};
-      (*timers)[std::string{key}] = entry.getValue();
+      timers[std::string(key)] = entry.getValue();
     }
   }
 
-  return Status::Ok();
+  return stats::StatsData(counters, timers);
 }
 
 void range_buffers_to_capnp(
@@ -246,11 +244,9 @@ Status subarray_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = subarray->stats();
-  if (stats != nullptr) {
-    auto stats_builder = builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = subarray->stats();
+  auto stats_builder = builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   if (subarray->relevant_fragments().relevant_fragments_size() > 0) {
     auto relevant_fragments_builder = builder->initRelevantFragments(
@@ -328,11 +324,8 @@ Status subarray_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader.hasStats()) {
-    stats::Stats* stats = subarray->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader.getStats());
+    subarray->set_stats(stats_data);
   }
 
   if (reader.hasRelevantFragments()) {
@@ -428,11 +421,9 @@ Status subarray_partitioner_to_capnp(
   builder->setMemoryBudgetValidity(mem_budget_validity);
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = partitioner.stats();
-  if (stats != nullptr) {
-    auto stats_builder = builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = partitioner.stats();
+  auto stats_builder = builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -566,11 +557,8 @@ Status subarray_partitioner_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader.hasStats()) {
-    auto stats = partitioner->stats();
-    // We should always have stats
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader.getStats());
+    partitioner->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -913,11 +901,9 @@ Status reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -947,11 +933,9 @@ Status index_reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -982,11 +966,9 @@ Status dense_reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -1146,11 +1128,8 @@ Status reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1187,11 +1166,8 @@ Status index_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1229,11 +1205,8 @@ Status dense_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1252,11 +1225,8 @@ Status delete_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (delete_reader.hasStats()) {
-    stats::Stats* stats = delete_strategy->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(delete_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(delete_reader.getStats());
+    delete_strategy->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1273,11 +1243,9 @@ Status delete_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = delete_strategy.stats();
-  if (stats != nullptr) {
-    auto stats_builder = delete_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *delete_strategy.stats();
+  auto stats_builder = delete_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -1301,11 +1269,9 @@ Status writer_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = writer.stats();
-  if (stats != nullptr) {
-    auto stats_builder = writer_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *writer.stats();
+  auto stats_builder = writer_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   if (query.layout() == Layout::GLOBAL_ORDER) {
     auto& global_writer = dynamic_cast<GlobalOrderWriter&>(writer);
@@ -1341,11 +1307,8 @@ Status writer_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (writer_reader.hasStats()) {
-    stats::Stats* stats = writer->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(writer_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(writer_reader.getStats());
+    writer->set_stats(stats_data);
   }
 
   if (query.layout() == Layout::GLOBAL_ORDER &&
@@ -1557,10 +1520,10 @@ Status query_to_capnp(
   RETURN_NOT_OK(config_to_capnp(query.config(), &config_builder));
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = query.stats();
-  if (stats != nullptr) {
+  auto stats = query.stats();
+  if (stats) {
     auto stats_builder = query_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
+    stats_to_capnp(*stats, &stats_builder);
   }
 
   auto& written_fragment_info = query.get_written_fragment_info();
@@ -2270,11 +2233,8 @@ Status query_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (query_reader.hasStats()) {
-    stats::Stats* stats = query->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(query_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(query_reader.getStats());
+    query->set_stats(stats_data);
   }
 
   if (query_reader.hasWrittenFragmentInfo()) {
@@ -3180,11 +3140,9 @@ void ordered_dim_label_reader_to_capnp(
       query.dimension_label_increasing_order());
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    throw_if_not_ok(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 }
 
 void ordered_dim_label_reader_from_capnp(
@@ -3212,11 +3170,8 @@ void ordered_dim_label_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      throw_if_not_ok(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 }