From f709bd994bab2300545427ce5d9b0cc78043c359 Mon Sep 17 00:00:00 2001
From: Rebekah Davis <bekadavis9@gmail.com>
Date: Thu, 22 Feb 2024 16:26:56 -0500
Subject: [PATCH] Address comments

---
 .../sm/consolidator/array_meta_consolidator.cc  | 15 +++++++--------
 .../sm/consolidator/group_meta_consolidator.cc  | 17 ++++++++---------
 tiledb/sm/metadata/metadata.cc                  |  8 ++++----
 tiledb/sm/metadata/metadata.h                   |  2 +-
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/tiledb/sm/consolidator/array_meta_consolidator.cc b/tiledb/sm/consolidator/array_meta_consolidator.cc
index 6397767aa45..cde5d10eb2c 100644
--- a/tiledb/sm/consolidator/array_meta_consolidator.cc
+++ b/tiledb/sm/consolidator/array_meta_consolidator.cc
@@ -65,7 +65,6 @@ Status ArrayMetaConsolidator::consolidate(
     const void* encryption_key,
     uint32_t key_length) {
   auto timer_se = stats_->start_timer("consolidate_array_meta");
-
   check_array_uri(array_name);
 
   // Open array for reading
@@ -86,9 +85,7 @@ Status ArrayMetaConsolidator::consolidate(
           QueryType::WRITE, encryption_type, encryption_key, key_length),
       throw_if_not_ok(array_for_reads.close()));
 
-  // "Swap" the in-memory metadata between the two arrays.
-  // After that, the array for writes will store the (consolidated by
-  // the way metadata loading works) metadata of the array for reads
+  // Copy-assign the read metadata into the metadata of the array for writes
   auto& metadata_r = array_for_reads.metadata();
   array_for_writes.opened_array()->metadata() = metadata_r;
   URI new_uri = metadata_r.get_uri(array_uri);
@@ -102,21 +99,23 @@ Status ArrayMetaConsolidator::consolidate(
     base_uri_size = array_for_reads.array_uri().to_string().size();
   }
 
-  // Write vacuum file
+  // Prepare vacuum file
   URI vac_uri = URI(new_uri.to_string() + constants::vacuum_file_suffix);
   std::stringstream ss;
   for (const auto& uri : to_vacuum) {
     ss << uri.to_string().substr(base_uri_size) << "\n";
   }
   auto data = ss.str();
-  RETURN_NOT_OK(
-      storage_manager_->vfs()->write(vac_uri, data.c_str(), data.size()));
-  RETURN_NOT_OK(storage_manager_->vfs()->close_file(vac_uri));
 
   // Close arrays
   throw_if_not_ok(array_for_reads.close());
   throw_if_not_ok(array_for_writes.close());
 
+  // Write vacuum file
+  RETURN_NOT_OK(
+      storage_manager_->vfs()->write(vac_uri, data.c_str(), data.size()));
+  RETURN_NOT_OK(storage_manager_->vfs()->close_file(vac_uri));
+
   return Status::Ok();
 }
 
diff --git a/tiledb/sm/consolidator/group_meta_consolidator.cc b/tiledb/sm/consolidator/group_meta_consolidator.cc
index 6ff4e975e23..ba0a74d5588 100644
--- a/tiledb/sm/consolidator/group_meta_consolidator.cc
+++ b/tiledb/sm/consolidator/group_meta_consolidator.cc
@@ -64,7 +64,6 @@ GroupMetaConsolidator::GroupMetaConsolidator(
 Status GroupMetaConsolidator::consolidate(
     const char* group_name, EncryptionType, const void*, uint32_t) {
   auto timer_se = stats_->start_timer("consolidate_group_meta");
-
   check_array_uri(group_name);
 
   // Open group for reading
@@ -81,29 +80,29 @@ Status GroupMetaConsolidator::consolidate(
       group_for_writes.open(QueryType::WRITE),
       throw_if_not_ok(group_for_reads.close()));
 
-  // "Swap" the in-memory metadata between the two groups.
-  // After that, the group for writes will store the (consolidated by
-  // the way metadata loading works) metadata of the group for reads
+  // Copy-assign the read metadata into the metadata of the group for writes
   auto metadata_r = group_for_reads.metadata();
   *(group_for_writes.metadata()) = *metadata_r;
   URI new_uri = metadata_r->get_uri(group_uri);
   const auto& to_vacuum = metadata_r->loaded_metadata_uris();
 
-  // Write vacuum file
+  // Prepare vacuum file
   URI vac_uri = URI(new_uri.to_string() + constants::vacuum_file_suffix);
   std::stringstream ss;
   for (const auto& uri : to_vacuum) {
     ss << uri.to_string() << "\n";
   }
   auto data = ss.str();
+
+  // Close groups
+  throw_if_not_ok(group_for_reads.close());
+  throw_if_not_ok(group_for_writes.close());
+
+  // Write vacuum file
   RETURN_NOT_OK(
       storage_manager_->vfs()->write(vac_uri, data.c_str(), data.size()));
   RETURN_NOT_OK(storage_manager_->vfs()->close_file(vac_uri));
 
-  // Close groups
-  RETURN_NOT_OK(group_for_reads.close());
-  RETURN_NOT_OK(group_for_writes.close());
-
   return Status::Ok();
 }
 
diff --git a/tiledb/sm/metadata/metadata.cc b/tiledb/sm/metadata/metadata.cc
index cc73d3c57f5..883b4237c15 100644
--- a/tiledb/sm/metadata/metadata.cc
+++ b/tiledb/sm/metadata/metadata.cc
@@ -80,8 +80,6 @@ Metadata::Metadata(
   build_metadata_index();
 }
 
-Metadata::~Metadata() = default;
-
 /* ********************************* */
 /*                API                */
 /* ********************************* */
@@ -139,9 +137,11 @@ void Metadata::generate_uri(const URI& array_uri) {
 
 std::map<std::string, Metadata::MetadataValue> Metadata::deserialize(
     const std::vector<shared_ptr<Tile>>& metadata_tiles) {
-  std::map<std::string, MetadataValue> metadata_map;
+  if (metadata_tiles.empty()) {
+    return {};
+  }
 
-  Status st;
+  std::map<std::string, MetadataValue> metadata_map;
   uint32_t key_len;
   char del;
   size_t value_len;
diff --git a/tiledb/sm/metadata/metadata.h b/tiledb/sm/metadata/metadata.h
index a88a51b9d76..8670ca9f152 100644
--- a/tiledb/sm/metadata/metadata.h
+++ b/tiledb/sm/metadata/metadata.h
@@ -104,7 +104,7 @@ class Metadata {
   DISABLE_MOVE_AND_MOVE_ASSIGN(Metadata);
 
   /** Destructor. */
-  ~Metadata();
+  ~Metadata() = default;
 
   /* ********************************* */
   /*                API                */