From e322ebffc6b3cf09902924884b7c2f702361aa90 Mon Sep 17 00:00:00 2001 From: Rebekah Davis Date: Mon, 18 Dec 2023 16:20:18 -0500 Subject: [PATCH] Address comments and format random number as 128-bit hex --- test/src/unit-cppapi-array.cc | 2 +- test/src/unit-cppapi-vfs.cc | 2 +- test/src/unit-s3-no-multipart.cc | 2 +- test/src/unit-s3.cc | 2 +- test/src/unit-vfs.cc | 10 +++--- test/support/src/helpers.h | 2 +- test/support/src/vfs_helpers.h | 6 ++-- tiledb/common/random/CMakeLists.txt | 4 +-- .../random/{helpers.cc => random_label.cc} | 33 ++++++++++++------- .../random/{helpers.h => random_label.h} | 17 +++++++--- .../random/test/unit_seedable_global_PRNG.cc | 14 ++++++-- 11 files changed, 60 insertions(+), 34 deletions(-) rename tiledb/common/random/{helpers.cc => random_label.cc} (61%) rename tiledb/common/random/{helpers.h => random_label.h} (71%) diff --git a/test/src/unit-cppapi-array.cc b/test/src/unit-cppapi-array.cc index a20c1e138e4..ade4203dbb4 100644 --- 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 index 96cbc0a59f8..ccdb8a165e5 100644 --- 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 index 38ef1694a3c..1965882ef24 100644 --- 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 index d02d51f8967..340ab95d433 100644 --- 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 index 860be803231..78ed7203b2b 100644 --- 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 index 1e86a2a68b2..f45054d4e42 100644 --- 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 index f083e731922..fea0bf0efa5 100644 --- 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 index cfb2ad6b807..edff498f639 100644 --- 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 b/tiledb/common/random/random_label.cc similarity index 61% rename from tiledb/common/random/helpers.cc rename to tiledb/common/random/random_label.cc index 45550593961..a16a028dd6b 100644 --- a/tiledb/common/random/helpers.cc +++ b/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 +#include + 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 \ No newline at end of file diff --git a/tiledb/common/random/helpers.h b/tiledb/common/random/random_label.h similarity index 71% rename from tiledb/common/random/helpers.h rename to tiledb/common/random/random_label.h index befad6d62db..00ef1fb773b 100644 --- a/tiledb/common/random/helpers.h +++ b/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 index c0b78e21ce7..b3d906207a8 100644 --- 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 #include "../prng.h" +#include "../random_label.h" #include "../seeder.h" -#include - 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); +}