fix: respect Appendix B: 32-bit Hash Requirements

zhjwpku · zhjwpku · commit d2af53018fa8 · 2025-09-20T22:34:15.000+08:00
diff --git a/src/iceberg/expression/literal.cc b/src/iceberg/expression/literal.cc
@@ -166,6 +166,15 @@ Result<Literal> Literal::Decimal(std::string_view value) {
   return Literal{Value{decimal_value.value()}, decimal(precision, scale)};
 }
 
+Literal Literal::UUID(std::array<uint8_t, 16> value) {
+  return {Value{std::move(value)}, uuid()};
+}
+
+Literal Literal::Fixed(std::vector<uint8_t> value) {
+  auto length = static_cast<int32_t>(value.size());
+  return {Value{std::move(value)}, fixed(length)};
+}
+
 Result<Literal> Literal::Deserialize(std::span<const uint8_t> data,
                                      std::shared_ptr<PrimitiveType> type) {
   Literal::Value value;
diff --git a/src/iceberg/expression/literal.h b/src/iceberg/expression/literal.h
@@ -19,7 +19,9 @@
 
 #pragma once
 
+#include <array>
 #include <compare>
+#include <cstdint>
 #include <memory>
 #include <string>
 #include <string_view>
@@ -76,6 +78,8 @@ class ICEBERG_EXPORT Literal {
   static Literal Binary(std::vector<uint8_t> value);
   static Literal Decimal(int128_t value, int32_t precision, int32_t scale);
   static Result<Literal> Decimal(std::string_view value);
+  static Literal UUID(std::array<uint8_t, 16> value);
+  static Literal Fixed(std::vector<uint8_t> value);
 
   /// \brief Create a literal representing a null value.
   static Literal Null(std::shared_ptr<PrimitiveType> type) {
diff --git a/src/iceberg/transform_function.cc b/src/iceberg/transform_function.cc
@@ -27,7 +27,11 @@
 
 #include "iceberg/expression/literal.h"
 #include "iceberg/type.h"
+#include "iceberg/type_fwd.h"
+#include "iceberg/util/decimal.h"
+#include "iceberg/util/endian.h"
 #include "iceberg/util/int128.h"
+#include "iceberg/util/macros.h"
 #include "iceberg/util/murmurhash3_internal.h"
 #include "iceberg/util/truncate_util.h"
 
@@ -54,7 +58,8 @@ BucketTransform::BucketTransform(std::shared_ptr<Type> const& source_type,
     : TransformFunction(TransformType::kBucket, source_type), num_buckets_(num_buckets) {}
 
 Result<Literal> BucketTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply bucket transform to literal with value {} of type {}",
@@ -69,17 +74,18 @@ Result<Literal> BucketTransform::Transform(const Literal& literal) {
       [&](auto&& value) {
         using T = std::decay_t<decltype(value)>;
         if constexpr (std::is_same_v<T, int32_t>) {
-          MurmurHash3_x86_32(&value, sizeof(int32_t), 0, &hash_value);
+          hash_value = HashInt(value);
         } else if constexpr (std::is_same_v<T, int64_t>) {
-          MurmurHash3_x86_32(&value, sizeof(int64_t), 0, &hash_value);
+          hash_value = HashLong(value);
         } else if constexpr (std::is_same_v<T, std::array<uint8_t, 16>>) {
-          MurmurHash3_x86_32(value.data(), sizeof(uint8_t) * 16, 0, &hash_value);
+          hash_value = HashBytes(value);
         } else if constexpr (std::is_same_v<T, int128_t>) {
-          MurmurHash3_x86_32(&value, sizeof(int128_t), 0, &hash_value);
+          hash_value = HashBytes(Decimal::ToBigEndian(value));
         } else if constexpr (std::is_same_v<T, std::string>) {
-          MurmurHash3_x86_32(value.data(), value.size(), 0, &hash_value);
+          hash_value = HashBytes(std::span<const uint8_t>(
+              reinterpret_cast<const uint8_t*>(value.data()), value.size()));
         } else if constexpr (std::is_same_v<T, std::vector<uint8_t>>) {
-          MurmurHash3_x86_32(value.data(), value.size(), 0, &hash_value);
+          hash_value = HashBytes(value);
         } else if constexpr (std::is_same_v<T, std::monostate> ||
                              std::is_same_v<T, bool> || std::is_same_v<T, float> ||
                              std::is_same_v<T, double> ||
@@ -129,12 +135,30 @@ Result<std::unique_ptr<TransformFunction>> BucketTransform::Make(
   return std::make_unique<BucketTransform>(source_type, num_buckets);
 }
 
+int32_t BucketTransform::HashBytes(std::span<const uint8_t> bytes) {
+  int32_t hash_value = 0;
+  MurmurHash3_x86_32(bytes.data(), bytes.size(), 0, &hash_value);
+  return hash_value;
+}
+
+int32_t BucketTransform::HashInt(int32_t value) {
+  return HashLong(static_cast<int64_t>(value));
+}
+
+int32_t BucketTransform::HashLong(int64_t value) {
+  int32_t hash_value = 0;
+  value = ToLittleEndian(value);
+  MurmurHash3_x86_32(&value, sizeof(int64_t), 0, &hash_value);
+  return hash_value;
+}
+
 TruncateTransform::TruncateTransform(std::shared_ptr<Type> const& source_type,
                                      int32_t width)
     : TransformFunction(TransformType::kTruncate, source_type), width_(width) {}
 
 Result<Literal> TruncateTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply truncate transform to literal with value {} of type {}",
@@ -205,7 +229,8 @@ YearTransform::YearTransform(std::shared_ptr<Type> const& source_type)
     : TransformFunction(TransformType::kTruncate, source_type) {}
 
 Result<Literal> YearTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply year transform to literal with value {} of type {}",
@@ -258,7 +283,8 @@ MonthTransform::MonthTransform(std::shared_ptr<Type> const& source_type)
     : TransformFunction(TransformType::kMonth, source_type) {}
 
 Result<Literal> MonthTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply month transform to literal with value {} of type {}",
@@ -323,7 +349,8 @@ DayTransform::DayTransform(std::shared_ptr<Type> const& source_type)
     : TransformFunction(TransformType::kDay, source_type) {}
 
 Result<Literal> DayTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply day transform to literal with value {} of type {}",
@@ -376,7 +403,8 @@ HourTransform::HourTransform(std::shared_ptr<Type> const& source_type)
     : TransformFunction(TransformType::kHour, source_type) {}
 
 Result<Literal> HourTransform::Transform(const Literal& literal) {
-  assert(literal.type() == source_type());
+  ICEBERG_DCHECK(*literal.type() == *source_type(),
+                 "Literal type must match source type");
   if (literal.IsBelowMin() || literal.IsAboveMax()) {
     return InvalidArgument(
         "Cannot apply hour transform to literal with value {} of type {}",
diff --git a/src/iceberg/transform_function.h b/src/iceberg/transform_function.h
@@ -51,6 +51,9 @@ class BucketTransform : public TransformFunction {
   BucketTransform(std::shared_ptr<Type> const& source_type, int32_t num_buckets);
 
   /// \brief Applies the bucket hash function to the input Literal.
+  ///
+  /// Reference:
+  /// - https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
   Result<Literal> Transform(const Literal& literal) override;
 
   /// \brief Returns INT32 as the output type.
@@ -63,6 +66,24 @@ class BucketTransform : public TransformFunction {
   static Result<std::unique_ptr<TransformFunction>> Make(
       std::shared_ptr<Type> const& source_type, int32_t num_buckets);
 
+  /// \brief Hash a byte array using MurmurHash3 and return a 32-bit hash value.
+  /// \param bytes The input byte array to hash.
+  /// \return A 32-bit hash value.
+  static int32_t HashBytes(std::span<const uint8_t> bytes);
+
+  /// \brief Hash a 32-bit integer using MurmurHash3 and return a 32-bit hash value.
+  /// \param value The input integer to hash.
+  /// \note Integer and long hash results must be identical for all integer values. This
+  /// ensures that schema evolution does not change bucket partition values if integer
+  /// types are promoted.
+  /// \return A 32-bit hash value.
+  static int32_t HashInt(int32_t value);
+
+  /// \brief Hash a 64-bit integer using MurmurHash3 and return a 32-bit hash value.
+  /// \param value The input long to hash.
+  /// \return A 32-bit hash value.
+  static int32_t HashLong(int64_t value);
+
  private:
   int32_t num_buckets_;
 };
diff --git a/src/iceberg/util/decimal.cc b/src/iceberg/util/decimal.cc
@@ -524,8 +524,8 @@ std::vector<uint8_t> Decimal::ToBigEndian(int128_t value) {
     // For negative numbers, keep the leading 0xff byte if the next byte has its sign bit
     // unset. For positive numbers, keep the leading 0x00 byte if the next byte has its
     // sign bit set.
-    if ((is_negative && byte == 0xff && (next & 0x80) == 0) ||
-        (!is_negative && byte == 0x00 && (next & 0x80) != 0)) {
+    if ((is_negative && byte == 0xff && (next & 0x80)) ||
+        (!is_negative && byte == 0x00 && !(next & 0x80))) {
       --keep;
     } else {
       break;
diff --git a/test/transform_test.cc b/test/transform_test.cc
@@ -19,14 +19,19 @@
 
 #include "iceberg/transform.h"
 
+#include <chrono>
 #include <format>
+#include <iostream>
 #include <memory>
+#include <string>
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include "iceberg/expression/literal.h"
+#include "iceberg/transform_function.h"
 #include "iceberg/type.h"
+#include "iceberg/util/decimal.h"
 #include "iceberg/util/formatter.h"  // IWYU pragma: keep
 #include "matchers.h"
 
@@ -241,10 +246,75 @@ TEST(TransformLiteralTest, IdentityTransform) {
   }
 }
 
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketTransformTest, HashHelper) {
+  // int and long
+  EXPECT_EQ(BucketTransform::HashInt(34), 2017239379);
+  EXPECT_EQ(BucketTransform::HashLong(34L), 2017239379);
+
+  // decimal hash
+  auto decimal = Decimal::FromString("14.20");
+  ASSERT_TRUE(decimal.has_value());
+  EXPECT_EQ(BucketTransform::HashBytes(Decimal::ToBigEndian(decimal->value())),
+            -500754589);
+
+  // date hash
+  // 2017-11-16
+  std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+  std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+  int32_t days = (sd - epoch).count();
+  std::cout << "days: " << days << std::endl;
+  EXPECT_EQ(BucketTransform::HashInt(days), -653330422);
+
+  // time
+  // 22:31:08 in microseconds
+  int64_t time_micros = (22 * 3600 + 31 * 60 + 8) * 1000000LL;
+  std::cout << "time micros: " << time_micros << std::endl;
+  EXPECT_EQ(BucketTransform::HashLong(time_micros), -662762989);
+
+  // timestamp
+  // 2017-11-16T22:31:08 in microseconds
+  std::chrono::system_clock::time_point tp =
+      std::chrono::sys_days{std::chrono::year{2017} / 11 / 16} + std::chrono::hours{22} +
+      std::chrono::minutes{31} + std::chrono::seconds{8};
+  int64_t timestamp_micros =
+      std::chrono::duration_cast<std::chrono::microseconds>(tp.time_since_epoch())
+          .count();
+  std::cout << "timestamp micros: " << timestamp_micros << std::endl;
+  EXPECT_EQ(BucketTransform::HashLong(timestamp_micros), -2047944441);
+  // 2017-11-16T22:31:08.000001 in microseconds
+  EXPECT_EQ(BucketTransform::HashLong(timestamp_micros + 1), -1207196810);
+
+  // string
+  std::string str = "iceberg";
+  EXPECT_EQ(BucketTransform::HashBytes(std::span<const uint8_t>(
+                reinterpret_cast<const uint8_t*>(str.data()), str.size())),
+            1210000089);
+
+  // uuid
+  // f79c3e09-677c-4bbd-a479-3f349cb785e7
+  std::array<uint8_t, 16> uuid = {0xf7, 0x9c, 0x3e, 0x09, 0x67, 0x7c, 0x4b, 0xbd,
+                                  0xa4, 0x79, 0x3f, 0x34, 0x9c, 0xb7, 0x85, 0xe7};
+  EXPECT_EQ(BucketTransform::HashBytes(uuid), 1488055340);
+
+  // fixed & binary
+  std::vector<uint8_t> fixed = {0, 1, 2, 3};
+  EXPECT_EQ(BucketTransform::HashBytes(fixed), -188683207);
+}
+
 TEST(TransformLiteralTest, BucketTransform) {
   constexpr int32_t num_buckets = 4;
   auto transform = Transform::Bucket(num_buckets);
 
+  // uuid
+  // f79c3e09-677c-4bbd-a479-3f349cb785e7
+  std::array<uint8_t, 16> uuid = {0xf7, 0x9c, 0x3e, 0x09, 0x67, 0x7c, 0x4b, 0xbd,
+                                  0xa4, 0x79, 0x3f, 0x34, 0x9c, 0xb7, 0x85, 0xe7};
+
+  // fixed & binary
+  std::vector<uint8_t> fixed = {0, 1, 2, 3};
+
   struct Case {
     std::shared_ptr<Type> source_type;
     Literal source;
@@ -253,23 +323,43 @@ TEST(TransformLiteralTest, BucketTransform) {
 
   const std::vector<Case> cases = {
       {.source_type = iceberg::int32(),
-       .source = Literal::Int(42),
+       .source = Literal::Int(34),
        .expected = Literal::Int(3)},
+      {.source_type = iceberg::int64(),
+       .source = Literal::Long(34),
+       .expected = Literal::Int(3)},
+      // decimal 14.20
+      {.source_type = iceberg::decimal(4, 2),
+       .source = Literal::Decimal(1420, 4, 2),
+       .expected = Literal::Int(3)},
+      // 2017-11-16
       {.source_type = iceberg::date(),
-       .source = Literal::Date(30000),
+       .source = Literal::Date(17486),
        .expected = Literal::Int(2)},
-      {.source_type = iceberg::int64(),
-       .source = Literal::Long(1234567890),
+      // // 22:31:08 in microseconds
+      {.source_type = iceberg::time(),
+       .source = Literal::Time(81068000000),
        .expected = Literal::Int(3)},
+      // // 2017-11-16T22:31:08 in microseconds
       {.source_type = iceberg::timestamp(),
-       .source = Literal::Timestamp(1622547800000000),
-       .expected = Literal::Int(1)},
+       .source = Literal::Timestamp(1510871468000000),
+       .expected = Literal::Int(3)},
+      // // 2017-11-16T22:31:08.000001 in microseconds
       {.source_type = iceberg::timestamp_tz(),
-       .source = Literal::TimestampTz(1622547800000000),
-       .expected = Literal::Int(1)},
+       .source = Literal::TimestampTz(1510871468000001),
+       .expected = Literal::Int(2)},
       {.source_type = iceberg::string(),
-       .source = Literal::String("test"),
-       .expected = Literal::Int(3)},
+       .source = Literal::String("iceberg"),
+       .expected = Literal::Int(1)},
+      {.source_type = iceberg::uuid(),
+       .source = Literal::UUID(uuid),
+       .expected = Literal::Int(0)},
+      {.source_type = iceberg::fixed(4),
+       .source = Literal::Fixed(fixed),
+       .expected = Literal::Int(1)},
+      {.source_type = iceberg::binary(),
+       .source = Literal::Binary(fixed),
+       .expected = Literal::Int(1)},
   };
 
   for (const auto& c : cases) {
