Copilot commented on code in PR #1019:
URL: https://github.com/apache/iceberg-cpp/pull/1019#discussion_r4281514002
##########
src/iceberg/location_provider.cc:
##########
@@ -89,9 +89,8 @@ std::string DirsFromHash(int32_t hash) {
}
std::string ComputeHash(std::string_view file_name) {
- int32_t hash_value = 0;
- MurmurHash3_x86_32(file_name.data(), file_name.size(), 0, &hash_value);
- return DirsFromHash(hash_value);
+ return DirsFromHash(
+ static_cast<int32_t>(MurmurHash3_x86_32(file_name.data(),
file_name.size(), 0)));
Review Comment:
Similar to bucket hashing, `static_cast<int32_t>` from the `uint32_t` hash
return value is implementation-defined for values with the high bit set.
Additionally, `DirsFromHash(int32_t)` (see context) uses right shifts on a
signed type, which is also implementation-defined for negative values. Consider
changing `DirsFromHash` to take `uint32_t` and using `uint32_t` throughout the
bit-extraction logic, and only converting to `int32_t` (via `std::bit_cast` if
needed) at APIs that truly require a signed type.
##########
src/iceberg/util/murmurhash3_internal.cc:
##########
@@ -2,443 +2,75 @@
// MurmurHash3 was written by Austin Appleby, and is placed in the public
// domain. The author hereby disclaims copyright to this source code.
-// Note - The x86 and x64 versions do _not_ produce the same results, as the
-// algorithms are optimized for their respective platforms. You can still
-// compile and run any of them on any platform, but your performance with the
-// non-native version will be less than optimal.
-
#include "iceberg/util/murmurhash3_internal.h"
-//-----------------------------------------------------------------------------
-// Platform-specific functions and macros
-
-// Microsoft Visual Studio
-
-#if defined(_MSC_VER)
-
-# define FORCE_INLINE __forceinline
-
-# include <cstdlib>
-
-# define ROTL32(x, y) _rotl(x, y)
-# define ROTL64(x, y) _rotl64(x, y)
-
-# define BIG_CONSTANT(x) (x)
+#include <bit>
-// Other compilers
+#include "iceberg/util/endian.h"
-#else // defined(_MSC_VER)
-
-# define FORCE_INLINE inline __attribute__((always_inline))
-
-inline uint32_t rotl32(uint32_t x, int8_t r) { return (x << r) | (x >> (32 -
r)); }
-
-inline uint64_t rotl64(uint64_t x, int8_t r) { return (x << r) | (x >> (64 -
r)); }
-
-# define ROTL32(x, y) rotl32(x, y)
-# define ROTL64(x, y) rotl64(x, y)
-
-# define BIG_CONSTANT(x) (x##LLU)
-
-#endif // !defined(_MSC_VER)
-
-// NOLINTBEGIN
+// Adapted from the SMHasher MurmurHash3 reference implementation:
+//
https://github.com/aappleby/smhasher/blob/07bb4de10a63e8cc2e1724865454eba635742383/src/MurmurHash3.cpp
namespace iceberg {
namespace {
-//-----------------------------------------------------------------------------
-// Block read - if your platform needs to do endian-swapping or can only
-// handle aligned reads, do the conversion here
-FORCE_INLINE uint32_t getblock32(const uint32_t* p, int i) { return p[i]; }
-
-FORCE_INLINE uint64_t getblock64(const uint64_t* p, int i) { return p[i]; }
-
-//-----------------------------------------------------------------------------
-// Finalization mix - force all bits of a hash block to avalanche
-
-FORCE_INLINE uint32_t fmix32(uint32_t h) {
- h ^= h >> 16;
- h *= 0x85ebca6b;
- h ^= h >> 13;
- h *= 0xc2b2ae35;
- h ^= h >> 16;
-
- return h;
+uint32_t MixBlock(uint32_t block) {
+ constexpr uint32_t kC1 = 0xcc9e2d51;
+ constexpr uint32_t kC2 = 0x1b873593;
+ block *= kC1;
+ block = std::rotl(block, 15);
+ return block * kC2;
}
-//----------
-
-FORCE_INLINE uint64_t fmix64(uint64_t k) {
- k ^= k >> 33;
- k *= BIG_CONSTANT(0xff51afd7ed558ccd);
- k ^= k >> 33;
- k *= BIG_CONSTANT(0xc4ceb9fe1a85ec53);
- k ^= k >> 33;
+uint32_t MixHash(uint32_t hash, uint32_t block) {
+ hash ^= MixBlock(block);
+ return std::rotl(hash, 13) * 5 + 0xe6546b64;
+}
- return k;
+uint32_t Finalize(uint32_t hash, size_t len) {
+ hash ^= static_cast<uint32_t>(len);
+ hash ^= hash >> 16;
+ hash *= 0x85ebca6b;
+ hash ^= hash >> 13;
+ hash *= 0xc2b2ae35;
+ hash ^= hash >> 16;
+ return hash;
}
Review Comment:
`MurmurHash3_x86_32` now accepts `size_t len`, but finalization truncates
`len` to 32 bits. That makes hashes for inputs larger than `UINT32_MAX` bytes
non-obvious/underspecified (and diverges from the reference API’s `int`
length). Suggest either (mandatory for clarity): (a) enforce `len <=
UINT32_MAX` with an explicit precondition check in `MurmurHash3_x86_32`, or (b)
change the API to take `uint32_t` length to match the algorithm’s definition
and avoid suggesting support for arbitrarily large buffers.
##########
src/iceberg/util/bucket_util.cc:
##########
@@ -119,28 +118,20 @@ int32_t BucketUtils::HashBytes(std::span<const uint8_t>
bytes) {
namespace internal {
int32_t HashBytes(const uint8_t* data, size_t len) {
- // MurmurHash3_x86_32 takes a signed 32-bit length. Passing a larger size_t
- // would wrap it negative and make the hash loop read far outside `data`
- // (and silently hash the wrong prefix for even larger inputs). Iceberg's
- // bucket transform is defined on byte sequences of at most 2^31 - 1 bytes
- // (Java arrays), so reject anything longer before narrowing.
+ // Keep bucket inputs compatible with Java's signed 32-bit array lengths.
+ // Reject oversized lengths before reading the input buffer.
ICEBERG_CHECK_OR_DIE(
len <= static_cast<size_t>(std::numeric_limits<int32_t>::max()),
"BucketUtils::HashBytes: input length {} exceeds the maximum supported "
"length {}",
len, std::numeric_limits<int32_t>::max());
- int32_t hash_value = 0;
- MurmurHash3_x86_32(data, static_cast<int>(len), 0, &hash_value);
- return hash_value;
+ return static_cast<int32_t>(MurmurHash3_x86_32(data, len, 0));
Review Comment:
`MurmurHash3_x86_32` now returns `uint32_t`, and converting that to
`int32_t` via `static_cast` is implementation-defined when the high bit is set
(values > `INT32_MAX`). Since this PR is explicitly about cross-platform hash
stability, prefer preserving the bit-pattern with
`std::bit_cast<int32_t>(uint32_t_hash)` (or an equivalent `memcpy`-based
approach if `bit_cast` isn’t available) so results are well-defined across
implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
