fix: make byte hashing independent of char signedness#361
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adjusts byte handling in hashing code paths to treat bytes as signed 8-bit values (consistent with Java/Hive semantics), improving hash compatibility across languages.
Changes:
- Update Hive hashing for raw bytes and TINYINT to use signed-byte interpretation.
- Update MurmurHash byte mixing and change
GetByteto returnint8_t.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/paimon/core/bucket/hive_hasher.h | Interprets char bytes as signed 8-bit when computing hash. |
| src/paimon/core/bucket/hive_bucket_function.cpp | Ensures TINYINT values are sign-extended before hashing. |
| src/paimon/common/utils/murmurhash_utils.h | Applies signed-byte mixing and changes GetByte return type to int8_t. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bb7fca9 to
59b29fb
Compare
- Treat Hive byte and byte-array hashing as signed Java bytes - Make Murmur tail-byte hashing use explicit int8_t values
|
Thanks for the fix! Could you add a compatibility test that compares the results with Java, covering negative tinyint values, high-byte UTF-8/binary data, and the Murmur unaligned tail-byte case? |
I would love to do this. But unfortunately I cannot build and run the test on my macOS laptop and I do not yet have access to the Java code. What I tried is to make sure it does not break existing CI and work as expected on our internal ARM environment. |
Purpose
Fix ARM bucket mismatches caused by platform-dependent
charsignedness.This change makes byte hashing explicit:
TINYINThashing now treats row bytes as signed Java bytes.int8_t, avoiding unsigned plain-chardrift.Tests
Pass existing CI.
API and Format
N/A
Documentation
N/A
Generative AI tooling
Generated-by: OpenAI Codex