Address comments and format random number as 128-bit hex

TileDB-Inc · Dec 18, 2023 · e322ebf · e322ebf
1 parent e709d67
commit e322ebf
Show file tree

Hide file tree

Showing 11 changed files with 60 additions and 34 deletions.
diff --git a/test/src/unit-cppapi-array.cc b/test/src/unit-cppapi-array.cc
@@ -1879,7 +1879,7 @@ TEST_CASE("C++ API: Array write and read from MemFS", "[cppapi][memfs]") {
 TEST_CASE(
     "C++ API: Array on s3 with empty subfolders",
     "[cppapi][s3][empty_subfolders]") {
-  const std::string array_bucket = "s3://" + random_label("tiledb") + "/";
+  const std::string array_bucket = "s3://" + random_label("tiledb-") + "/";
   const std::string array_name = array_bucket + "cpp_unit_array/";
 
   tiledb::Config cfg;

diff --git a/test/src/unit-cppapi-vfs.cc b/test/src/unit-cppapi-vfs.cc
@@ -479,7 +479,7 @@ TEST_CASE(
       tiledb_ctx_is_supported_fs(ctx.ptr().get(), TILEDB_S3, &s3) == TILEDB_OK);
   if (s3) {
     tiledb::VFS vfs(ctx);
-    std::string bucket_name = "s3://" + random_label("tiledb") + "/";
+    std::string bucket_name = "s3://" + random_label("tiledb-") + "/";
     if (vfs.is_bucket(bucket_name)) {
       REQUIRE_NOTHROW(vfs.remove_bucket(bucket_name));
     }

diff --git a/test/src/unit-s3-no-multipart.cc b/test/src/unit-s3-no-multipart.cc
@@ -54,7 +54,7 @@ struct S3DirectFx {
 
   const std::string S3_PREFIX = "s3://";
   const tiledb::sm::URI S3_BUCKET =
-      tiledb::sm::URI(S3_PREFIX + random_label("tiledb") + "/");
+      tiledb::sm::URI(S3_PREFIX + random_label("tiledb-") + "/");
   const std::string TEST_DIR = S3_BUCKET.to_string() + "tiledb_test_dir/";
   ThreadPool thread_pool_{2};
   tiledb::sm::S3 s3_{&g_helper_stats, &thread_pool_, set_config_params()};

diff --git a/test/src/unit-s3.cc b/test/src/unit-s3.cc
@@ -54,7 +54,7 @@ struct S3Fx {
 
   const std::string S3_PREFIX = "s3://";
   const tiledb::sm::URI S3_BUCKET =
-      tiledb::sm::URI(S3_PREFIX + random_label("tiledb") + "/");
+      tiledb::sm::URI(S3_PREFIX + random_label("tiledb-") + "/");
   const std::string TEST_DIR = S3_BUCKET.to_string() + "tiledb_test_dir/";
   ThreadPool thread_pool_{2};
   tiledb::sm::S3 s3_{&g_helper_stats, &thread_pool_, set_config_params()};

diff --git a/test/src/unit-vfs.cc b/test/src/unit-vfs.cc
@@ -154,12 +154,12 @@ TEST_CASE("VFS: URI semantics", "[vfs][uri]") {
     REQUIRE(config.set("vfs.s3.verify_ssl", "false").ok());
 
     root_pairs.emplace_back(
-        URI("s3://" + random_label("vfs") + "/"), std::move(config));
+        URI("s3://" + random_label("vfs-") + "/"), std::move(config));
   }
   if constexpr (tiledb::sm::filesystem::hdfs_enabled) {
     Config config;
     root_pairs.emplace_back(
-        URI("hdfs:///" + random_label("vfs") + "/"), std::move(config));
+        URI("hdfs:///" + random_label("vfs-") + "/"), std::move(config));
   }
   if constexpr (tiledb::sm::filesystem::azure_enabled) {
     Config config;
@@ -179,17 +179,17 @@ TEST_CASE("VFS: URI semantics", "[vfs][uri]") {
                 .ok());
 
     root_pairs.emplace_back(
-        URI("azure://" + random_label("vfs") + "/"), std::move(config));
+        URI("azure://" + random_label("vfs-") + "/"), std::move(config));
   }
 
   Config config;
 #ifdef _WIN32
   root_pairs.emplace_back(
-      URI(tiledb::sm::Win::current_dir() + "\\" + random_label("vfs") + "\\"),
+      URI(tiledb::sm::Win::current_dir() + "\\" + random_label("vfs-") + "\\"),
       std::move(config));
 #else
   root_pairs.emplace_back(
-      URI(Posix::current_dir() + "/" + random_label("vfs") + "/"),
+      URI(Posix::current_dir() + "/" + random_label("vfs-") + "/"),
       std::move(config));
 #endif
 

diff --git a/test/support/src/helpers.h b/test/support/src/helpers.h
@@ -37,7 +37,7 @@
 #include "test/support/src/coords_workaround.h"
 #include "tiledb.h"
 #include "tiledb/common/common.h"
-#include "tiledb/common/random/helpers.h"
+#include "tiledb/common/random/random_label.h"
 #include "tiledb/sm/array/array.h"
 #include "tiledb/sm/cpp_api/tiledb"
 #include "tiledb/sm/enums/layout.h"

diff --git a/test/support/src/vfs_helpers.h b/test/support/src/vfs_helpers.h
@@ -148,7 +148,7 @@ class SupportedFsS3 : public SupportedFs {
  public:
   SupportedFsS3()
       : s3_prefix_("s3://")
-      , s3_bucket_(s3_prefix_ + random_label("tiledb") + "/")
+      , s3_bucket_(s3_prefix_ + random_label("tiledb-") + "/")
       , temp_dir_(s3_bucket_ + "tiledb_test/") {
   }
 
@@ -275,7 +275,7 @@ class SupportedFsAzure : public SupportedFs {
  public:
   SupportedFsAzure()
       : azure_prefix_("azure://")
-      , container_(azure_prefix_ + random_label("tiledb") + "/")
+      , container_(azure_prefix_ + random_label("tiledb-") + "/")
       , temp_dir_(container_ + "tiledb_test/") {
   }
 
@@ -342,7 +342,7 @@ class SupportedFsGCS : public SupportedFs {
  public:
   SupportedFsGCS(std::string prefix = "gcs://")
       : prefix_(prefix)
-      , bucket_(prefix_ + random_label("tiledb") + "/")
+      , bucket_(prefix_ + random_label("tiledb-") + "/")
       , temp_dir_(bucket_ + "tiledb_test/") {
   }
 

diff --git a/tiledb/common/random/CMakeLists.txt b/tiledb/common/random/CMakeLists.txt
@@ -28,8 +28,8 @@ include(common NO_POLICY_SCOPE)
 include(object_library)
 
 list(APPEND SOURCES
-  helpers.cc  
-  prng.cc    
+  prng.cc
+  random_label.cc
   seeder.cc
 )
 gather_sources(${SOURCES})

diff --git a/tiledb/common/random/helpers.cc → tiledb/common/random/random_label.cc b/tiledb/common/random/helpers.cc → tiledb/common/random/random_label.cc
@@ -1,5 +1,5 @@
 /**
- * @file   helpers.cc
+ * @file   random_label.cc
  *
  * @section LICENSE
  *
@@ -27,26 +27,37 @@
  *
  * @section DESCRIPTION
  *
- * This file defines some random helper functions.
+ * This file defines a random label generator.
  */
 
-#include "tiledb/common/random/helpers.h"
+#include "tiledb/common/random/random_label.h"
 #include "tiledb/common/random/prng.h"
 
+#include <iomanip>
+#include <sstream>
+
 namespace tiledb::common {
 
+/**
+ * Legacy code provides randomness using UUIDs, which are always 128 bits,
+ * represented as a 32-digit hexadecimal value.
+ *
+ * To ensure backward compatibility, this function formats the PRNG-generated
+ * values to be precisely a 32-digit hexadecimal value. Each value is padded
+ * with 0s such that it makes up one 16-digit half of the full 32-digit number.
+ */
 std::string random_label(std::string prefix) {
-  // Generate random number using global PRNG
   PRNG& prng = PRNG::get();
-  auto rand = prng();
+  std::stringstream ss;
 
-  // Ensure prefix ends with "-"
-  if (!prefix.ends_with("-")) {
-    prefix = prefix + "-";
-  }
+  // Generate and format a 128-bit, 32-digit hexadecimal random number
+  auto rand1 = prng();
+  ss << std::hex << std::setw(16) << std::setfill('0') << rand1;
+  auto rand2 = prng();
+  ss << std::hex << std::setw(16) << std::setfill('0') << rand2;
 
-  // Generate random label
-  return prefix + std::to_string(rand);
+  // Return label string
+  return prefix + ss.str();
 }
 
 }  // namespace tiledb::common
diff --git a/tiledb/common/random/helpers.h → tiledb/common/random/random_label.h b/tiledb/common/random/helpers.h → tiledb/common/random/random_label.h
@@ -1,5 +1,5 @@
 /**
- * @file   helpers.h
+ * @file   random_label.h
  *
  * @section LICENSE
  *
@@ -27,7 +27,7 @@
  *
  * @section DESCRIPTION
  *
- * This file declares some random helper functions.
+ * This file declares a random label generator.
  */
 
 #ifndef TILEDB_HELPERS_H
@@ -38,12 +38,19 @@
 namespace tiledb::common {
 
 /**
- * Returns a random label, with given prefix and a random number as suffix.
+ * Returns a PRNG-generated random label with the optionally-provided prefix.
  *
- * @param prefix The prefix of the label.
+ * Given prefix "tiledb-", this function will return a label with syntax
+ * tiledb-<32-bit hexadecimal random number>.
+ * (Ex. tiledb-f258d22d4db9139204eef2b4b5d860cc).
+ *
+ * Note: the random number is actually the combination of two 16-bit numbers.
+ * The values are 0-padded to ensure exactly a 32-bit length.
+ *
+ * @param prefix The optional prefix of the label.
  * @return A random label.
  */
-std::string random_label(std::string prefix);
+std::string random_label(std::string prefix = "");
 
 }  // namespace tiledb::common
 

diff --git a/tiledb/common/random/test/unit_seedable_global_PRNG.cc b/tiledb/common/random/test/unit_seedable_global_PRNG.cc
@@ -30,10 +30,9 @@
 
 #include <test/support/tdb_catch.h>
 #include "../prng.h"
+#include "../random_label.h"
 #include "../seeder.h"
 
-#include <iostream>
-
 using namespace tiledb::common;
 
 TEST_CASE(
@@ -96,7 +95,7 @@ TEST_CASE(
 TEST_CASE(
     "SeedableGlobalPRNG: operator",
     "[SeedableGlobalPRNG][operator][multiple]") {
-  PRNG prng;
+  PRNG& prng = PRNG::get();
   auto rand_num1 = prng();
   CHECK(rand_num1 != 0);
 
@@ -128,3 +127,12 @@ TEST_CASE(
         Catch::Matchers::ContainsSubstring("Seed can only be used once"));
   }
 }
+
+TEST_CASE("random_label", "[random_label]") {
+  auto rand_label1 = random_label();
+  CHECK(rand_label1.length() == 32);
+
+  auto rand_label2 = random_label();
+  CHECK(rand_label2.length() == 32);
+  CHECK(rand_label1 != rand_label2);
+}