fix: review comments

zhjwpku · zhjwpku · commit b435dc9bbb3c · 2025-09-25T22:59:23.000+08:00
diff --git a/src/iceberg/util/uuid.cc b/src/iceberg/util/uuid.cc
@@ -32,77 +32,6 @@
 
 namespace iceberg {
 
-Uuid::Uuid(std::array<uint8_t, kUuidSize> data) : data_(std::move(data)) {}
-
-Uuid Uuid::GenerateV4() {
-  static std::random_device rd;
-  static std::mt19937 gen(rd());
-  static std::uniform_int_distribution<uint64_t> distrib(
-      std::numeric_limits<uint64_t>::min(), std::numeric_limits<uint64_t>::max());
-  std::array<uint8_t, 16> uuid;
-
-  // Generate two random 64-bit integers
-  uint64_t high_bits = distrib(gen);
-  uint64_t low_bits = distrib(gen);
-
-  // Combine them into a uint128_t
-  uint128_t random_128_bit_number = (static_cast<uint128_t>(high_bits) << 64) | low_bits;
-
-  // Copy the bytes into the uuid array
-  std::memcpy(uuid.data(), &random_128_bit_number, 16);
-
-  // Set magic numbers for a "version 4" (pseudorandom) UUID and variant,
-  // see https://datatracker.ietf.org/doc/html/rfc9562#name-uuid-version-4
-  uuid[6] = (uuid[6] & 0x0F) | 0x40;
-  // Set variant field, top two bits are 1, 0
-  uuid[8] = (uuid[8] & 0x3F) | 0x80;
-
-  return Uuid(std::move(uuid));
-}
-
-Uuid Uuid::GenerateV7() {
-  // Get the current time in milliseconds since the Unix epoch
-  auto now = std::chrono::system_clock::now();
-  auto duration_since_epoch = now.time_since_epoch();
-  auto unix_ts_ms =
-      std::chrono::duration_cast<std::chrono::milliseconds>(duration_since_epoch).count();
-
-  return GenerateV7(static_cast<uint64_t>(unix_ts_ms));
-}
-
-Uuid Uuid::GenerateV7(uint64_t unix_ts_ms) {
-  std::array<uint8_t, 16> uuid = {};
-
-  // Set the timestamp (in milliseconds since Unix epoch)
-  uuid[0] = (unix_ts_ms >> 40) & 0xFF;
-  uuid[1] = (unix_ts_ms >> 32) & 0xFF;
-  uuid[2] = (unix_ts_ms >> 24) & 0xFF;
-  uuid[3] = (unix_ts_ms >> 16) & 0xFF;
-  uuid[4] = (unix_ts_ms >> 8) & 0xFF;
-  uuid[5] = unix_ts_ms & 0xFF;
-
-  // Generate random bytes for the remaining fields
-  static std::random_device rd;
-  static std::mt19937 gen(rd());
-  static std::uniform_int_distribution<uint16_t> distrib(
-      std::numeric_limits<uint16_t>::min(), std::numeric_limits<uint16_t>::max());
-
-  // Note: uint8_t is invalid for uniform_int_distribution on Windows
-  for (size_t i = 6; i < 16; i += 2) {
-    auto rand = static_cast<uint16_t>(distrib(gen));
-    uuid[i] = (rand >> 8) & 0xFF;
-    uuid[i + 1] = rand & 0xFF;
-  }
-
-  // Set magic numbers for a "version 7" (pseudorandom) UUID and variant,
-  // see https://www.rfc-editor.org/rfc/rfc9562#name-version-field
-  uuid[6] = (uuid[6] & 0x0F) | 0x70;
-  // set variant field, top two bits are 1, 0
-  uuid[8] = (uuid[8] & 0x3F) | 0x80;
-
-  return Uuid(std::move(uuid));
-}
-
 namespace {
 
 constexpr std::array<uint8_t, 256> BuildHexTable() {
@@ -182,6 +111,77 @@ inline Result<Uuid> ParseHyphenated(std::string_view s) {
 
 }  // namespace
 
+Uuid::Uuid(std::array<uint8_t, kLength> data) : data_(std::move(data)) {}
+
+Uuid Uuid::GenerateV4() {
+  static std::random_device rd;
+  static std::mt19937 gen(rd());
+  static std::uniform_int_distribution<uint64_t> distrib(
+      std::numeric_limits<uint64_t>::min(), std::numeric_limits<uint64_t>::max());
+  std::array<uint8_t, 16> uuid;
+
+  // Generate two random 64-bit integers
+  uint64_t high_bits = distrib(gen);
+  uint64_t low_bits = distrib(gen);
+
+  // Combine them into a uint128_t
+  uint128_t random_128_bit_number = (static_cast<uint128_t>(high_bits) << 64) | low_bits;
+
+  // Copy the bytes into the uuid array
+  std::memcpy(uuid.data(), &random_128_bit_number, 16);
+
+  // Set magic numbers for a "version 4" (pseudorandom) UUID and variant,
+  // see https://datatracker.ietf.org/doc/html/rfc9562#name-uuid-version-4
+  uuid[6] = (uuid[6] & 0x0F) | 0x40;
+  // Set variant field, top two bits are 1, 0
+  uuid[8] = (uuid[8] & 0x3F) | 0x80;
+
+  return Uuid(std::move(uuid));
+}
+
+Uuid Uuid::GenerateV7() {
+  // Get the current time in milliseconds since the Unix epoch
+  auto now = std::chrono::system_clock::now();
+  auto duration_since_epoch = now.time_since_epoch();
+  auto unix_ts_ms =
+      std::chrono::duration_cast<std::chrono::milliseconds>(duration_since_epoch).count();
+
+  return GenerateV7(static_cast<uint64_t>(unix_ts_ms));
+}
+
+Uuid Uuid::GenerateV7(uint64_t unix_ts_ms) {
+  std::array<uint8_t, 16> uuid = {};
+
+  // Set the timestamp (in milliseconds since Unix epoch)
+  uuid[0] = (unix_ts_ms >> 40) & 0xFF;
+  uuid[1] = (unix_ts_ms >> 32) & 0xFF;
+  uuid[2] = (unix_ts_ms >> 24) & 0xFF;
+  uuid[3] = (unix_ts_ms >> 16) & 0xFF;
+  uuid[4] = (unix_ts_ms >> 8) & 0xFF;
+  uuid[5] = unix_ts_ms & 0xFF;
+
+  // Generate random bytes for the remaining fields
+  static std::random_device rd;
+  static std::mt19937 gen(rd());
+  static std::uniform_int_distribution<uint16_t> distrib(
+      std::numeric_limits<uint16_t>::min(), std::numeric_limits<uint16_t>::max());
+
+  // Note: uint8_t is invalid for uniform_int_distribution on Windows
+  for (size_t i = 6; i < 16; i += 2) {
+    auto rand = static_cast<uint16_t>(distrib(gen));
+    uuid[i] = (rand >> 8) & 0xFF;
+    uuid[i + 1] = rand & 0xFF;
+  }
+
+  // Set magic numbers for a "version 7" (pseudorandom) UUID and variant,
+  // see https://www.rfc-editor.org/rfc/rfc9562#name-version-field
+  uuid[6] = (uuid[6] & 0x0F) | 0x70;
+  // set variant field, top two bits are 1, 0
+  uuid[8] = (uuid[8] & 0x3F) | 0x80;
+
+  return Uuid(std::move(uuid));
+}
+
 Result<Uuid> Uuid::FromString(std::string_view str) {
   if (str.size() == 32) {
     return ParseSimple(str);
@@ -193,23 +193,23 @@ Result<Uuid> Uuid::FromString(std::string_view str) {
 }
 
 Result<Uuid> Uuid::FromBytes(std::span<const uint8_t> bytes) {
-  if (bytes.size() != kUuidSize) [[unlikely]] {
-    return InvalidArgument("UUID byte array must be exactly {} bytes, was {}", kUuidSize,
+  if (bytes.size() != kLength) [[unlikely]] {
+    return InvalidArgument("UUID byte array must be exactly {} bytes, was {}", kLength,
                            bytes.size());
   }
-  std::array<uint8_t, kUuidSize> data;
-  std::memcpy(data.data(), bytes.data(), kUuidSize);
+  std::array<uint8_t, kLength> data;
+  std::memcpy(data.data(), bytes.data(), kLength);
   return Uuid(std::move(data));
 }
 
 uint8_t Uuid::operator[](size_t index) const {
-  ICEBERG_CHECK(index < kUuidSize, "UUID index out of range: {}", index);
+  ICEBERG_CHECK(index < kLength, "UUID index out of range: {}", index);
   return data_[index];
 }
 
-std::string Uuid::ToString() const {
-  static const char* hex_chars = "0123456789abcdef";
+std::array<uint8_t, Uuid::kLength> Uuid::ToBigEndianBytes() const { return data_; }
 
+std::string Uuid::ToString() const {
   return std::format(
       "{:02x}{:02x}{:02x}{:02x}-{:02x}{:02x}-{:02x}{:02x}-{:02x}{:02x}-{:02x}{:02x}{:02x}"
       "{:02x}{:02x}{:02x}",
diff --git a/src/iceberg/util/uuid.h b/src/iceberg/util/uuid.h
@@ -35,9 +35,9 @@ namespace iceberg {
 class ICEBERG_EXPORT Uuid {
  public:
   Uuid() = delete;
-  constexpr static size_t kUuidSize = 16;
+  constexpr static size_t kLength = 16;
 
-  explicit Uuid(std::array<uint8_t, kUuidSize> data);
+  explicit Uuid(std::array<uint8_t, kLength> data);
 
   /// \brief Generate a random UUID (version 4).
   static Uuid GenerateV4();
@@ -61,10 +61,16 @@ class ICEBERG_EXPORT Uuid {
   /// \brief Create a UUID from a 16-byte array.
   static Result<Uuid> FromBytes(std::span<const uint8_t> bytes);
 
+  /// \brief Get the raw bytes of the UUID in big-endian order.
+  std::array<uint8_t, kLength> ToBigEndianBytes() const;
+
   /// \brief Get the raw bytes of the UUID.
   std::span<const uint8_t> bytes() const { return data_; }
 
   /// \brief Access individual bytes of the UUID.
+  /// \param index The index of the byte to access (0-15).
+  /// \return The byte at the specified index.
+  /// \throw IcebergError if index is out of bounds.
   uint8_t operator[](size_t index) const;
 
   /// \brief Convert the UUID to a string in standard format.
@@ -75,7 +81,7 @@ class ICEBERG_EXPORT Uuid {
   }
 
  private:
-  std::array<uint8_t, kUuidSize> data_;
+  std::array<uint8_t, kLength> data_;
 };
 
 }  // namespace iceberg
diff --git a/test/uuid_test.cc b/test/uuid_test.cc
@@ -30,7 +30,7 @@ namespace iceberg {
 TEST(UUIDUtilTest, GenerateV4) {
   auto uuid = Uuid::GenerateV4();
   // just ensure it runs and produces a value
-  EXPECT_EQ(uuid.bytes().size(), Uuid::kUuidSize);
+  EXPECT_EQ(uuid.bytes().size(), Uuid::kLength);
   // Version 4 UUIDs have the version number (4) in the 7th byte
   EXPECT_EQ((uuid[6] >> 4) & 0x0F, 4);
   // Variant is in the 9th byte, the two most significant bits should be 10
@@ -95,9 +95,9 @@ TEST(UUIDUtilTest, FromStringInvalid) {
 }
 
 TEST(UUIDUtilTest, FromBytes) {
-  std::array<uint8_t, Uuid::kUuidSize> bytes = {0x12, 0x3e, 0x45, 0x67, 0xe8, 0x9b,
-                                                0x12, 0xd3, 0xa4, 0x56, 0x42, 0x66,
-                                                0x14, 0x17, 0x40, 0x00};
+  std::array<uint8_t, Uuid::kLength> bytes = {0x12, 0x3e, 0x45, 0x67, 0xe8, 0x9b,
+                                              0x12, 0xd3, 0xa4, 0x56, 0x42, 0x66,
+                                              0x14, 0x17, 0x40, 0x00};
   auto result = Uuid::FromBytes(bytes);
   EXPECT_THAT(result, IsOk());
   auto uuid = result.value();
@@ -106,9 +106,9 @@ TEST(UUIDUtilTest, FromBytes) {
 }
 
 TEST(UUIDUtilTest, FromBytesInvalid) {
-  std::array<uint8_t, Uuid::kUuidSize - 1> short_bytes = {0x12, 0x3e, 0x45, 0x67, 0xe8,
-                                                          0x9b, 0x12, 0xd3, 0xa4, 0x56,
-                                                          0x42, 0x66, 0x14, 0x17, 0x40};
+  std::array<uint8_t, Uuid::kLength - 1> short_bytes = {0x12, 0x3e, 0x45, 0x67, 0xe8,
+                                                        0x9b, 0x12, 0xd3, 0xa4, 0x56,
+                                                        0x42, 0x66, 0x14, 0x17, 0x40};
   auto result = Uuid::FromBytes(short_bytes);
   EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
   EXPECT_THAT(result, HasErrorMessage("UUID byte array must be exactly 16 bytes"));
