Copilot commented on code in PR #1019:
URL: https://github.com/apache/iceberg-cpp/pull/1019#discussion_r4279171945
##########
src/iceberg/util/murmurhash3_internal.cc:
##########
@@ -2,443 +2,71 @@
// 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)
-
-// Other compilers
-
-#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)
+#include <bit>
-// NOLINTBEGIN
+#include "iceberg/util/endian.h"
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;
}
} // namespace
-//-----------------------------------------------------------------------------
-
-void MurmurHash3_x86_32(const void* key, int len, uint32_t seed, void* out) {
- const uint8_t* data = (const uint8_t*)key;
- const int nblocks = len / 4;
-
- uint32_t h1 = seed;
-
- const uint32_t c1 = 0xcc9e2d51;
- const uint32_t c2 = 0x1b873593;
-
- //----------
- // body
-
- const uint32_t* blocks = (const uint32_t*)(data + nblocks * 4);
-
- for (int i = -nblocks; i; i++) {
- uint32_t k1 = getblock32(blocks, i);
-
- k1 *= c1;
- k1 = ROTL32(k1, 15);
- k1 *= c2;
+uint32_t MurmurHash3_x86_32(const void* key, size_t len, uint32_t seed) {
+ const auto* data = static_cast<const uint8_t*>(key);
+ const size_t nblocks = len / sizeof(uint32_t);
Review Comment:
The new runtime vectors stop at 33 bytes, so they do not exercise the
length-narrowing bug this change fixes. The signature assertion would still
pass if `HashBytes` narrowed `bytes.size()` to `int`. Add the described 2 GiB
regression through `BucketUtils::HashBytes`, check an independently verified
golden hash, and skip when the mapping is unavailable.
--
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]
