Repository navigation
fix(bucket): use endian-safe MurmurHash block loads - #1019
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open findings
What changed in this PR
This PR makes MurmurHash3 block loads endian-safe and alignment-safe to ensure stable bucket hashing across architectures (notably big-endian hosts), and adds regression/golden-vector tests to lock in canonical results.
Changes:
- Replace MurmurHash3 block reads with
ReadLittleEndianand switch block pointers to byte-addressed (uint8_t*) across x86_32, x86_128, and x64_128 variants. - Add canonical SMHasher-derived golden vectors (including offset 0–7 and block/tail boundary coverage) for all three MurmurHash3 variants.
- Add canonical bucket-value tests for multiple literal types (int/long/string/binary/fixed/UUID) and register the new test in
util_test.
| File | Description |
|---|---|
| src/iceberg/util/murmurhash3_internal.cc | Uses endian-safe, unaligned-safe little-endian loads for MurmurHash3 blocks. |
| src/iceberg/test/murmurhash3_internal_test.cc | Adds golden-vector tests (SMHasher fixtures) covering offsets, lengths, and seeds. |
| src/iceberg/test/bucket_util_test.cc | Adds canonical bucket regression vectors for multiple literal input types. |
| src/iceberg/test/CMakeLists.txt | Registers the new MurmurHash3 internal test in the util test target. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
de3ca1e to
ff28fc0
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
🧠 Review effort: Lite
Read unaligned blocks in canonical little-endian order, accept size_t lengths, and mix integer halves directly. Keep path entropy on the raw helper and preserve upstream bucket input validation. Cover canonical byte hashes, integer encoding, byte literal buckets, and path prefixes with small regressions.
ff28fc0 to
40640a6
Compare
| h1 = ROTL32(h1, 13); | ||
| h1 = h1 * 5 + 0xe6546b64; | ||
| for (size_t i = 0; i < nblocks; ++i) { | ||
| hash = MixHash(hash, ReadLittleEndian<uint32_t>(data)); |
There was a problem hiding this comment.
Checked the new path against the SMHasher reference and it's bit-identical, and _Long matches Guava's hashLong. No CI leg is big-endian though, so the fix this PR is named for stays unverified - the new tests only have teeth on the sanitizer job.
There was a problem hiding this comment.
I intentionally leave it out of this PR since it's a CI change and might need more discussion. Besides, I've also checked no other iceberg projects have set up a big-endian host CI. It could be a follow up if we decide to add one.
Summary
Native word loads in MurmurHash3 produce incorrect Iceberg bucket hashes on big-endian hosts and rely on aligned input. Read blocks safely in little-endian order so bucket values match the Iceberg specification across architectures.
size_tlengths, return the numeric hash directly, and remove the unused 128-bit variants.HashBytesretains theINT32_MAXlimit for Java compatibility, andBucketIndexreturnsInvalidArgumentfor oversized string, binary, or fixed values before hashing. Correct the comments to distinguish this compatibility limit from the raw helper'ssize_tlength.Part of #1000. Squashed into one commit on upstream
mainat60e67fa.Compatibility
Existing bucketed data written by older C++ libraries on big-endian hosts requires migration. For int/long
34, the hash changes from1403955681to2017239379, andbucket[100]changes from81to79. After upgrading, bucket partition pruning can skip files with the old assignment and silently omit matching rows.Read affected tables with an unfiltered scan, apply row filters after reading, and rewrite the data into corrected buckets with matching partition metadata before relying on filtered scans. A filtered rewrite can itself omit affected files. There is no hash-version marker, legacy-hash compatibility mode, or automatic migration.
Little-endian hashes remain compatible within the supported input lengths. Newly generated object-store entropy prefixes can change on big-endian hosts; existing files retain their manifest-recorded paths.
Validation
git diff --checkpassed.bucket_util_test.ccand one focused filename-prefix check passed in a standalone binary linked with the production sources: under 0.01 seconds and about 5 MiB peak resident memory.noexceptand rejectedintanduint32_tlength parameters on the local 64-bit build.The full local utility/table suite was not run.