fix: platform-stable Hash for Seq, SeqSlice, and Kmer (#15) - #16
Merged
Conversation
Hash for SeqSlice and Kmer fed `usize`-typed lengths and bitvec's per-bit Hash to the hasher, producing different digests on 32- vs. 64-bit targets. Replace both impls with an explicit u64 LE length prefix and a `hash_bits` helper that extracts whole bytes via `BitField::load_le::<u8>`, sidestepping bitvec's Hash and the usize storage width. Truncate Kmer's backing bitarray to K * A::BITS so storage padding can't leak into the digest. Tests use a RecordingHasher to capture the byte stream and assert contract-level invariants (cross-route equivalence between Seq / &SeqSlice / Kmer, structural length, padding-independence across storage widths) over a curated set of codecs and lengths.
werner291
force-pushed
the
fix/15-platform-stable-hash
branch
from
April 30, 2026 19:04
19ef947 to
bea4368
Compare
jeff-k
approved these changes
May 17, 2026
Comment on lines
+1
to
+5
| // Copyright 2021-2024 Jeff Knaggs | ||
| // Licensed under the MIT license (http://opensource.org/licenses/MIT) | ||
| // This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
| let mut buf = [0u8; 64]; | ||
| let mut len = 0; | ||
| for chunk in bs.chunks(8) { | ||
| buf[len] = chunk.load_le::<u8>(); |
Owner
There was a problem hiding this comment.
I wonder if it might make sense to always use u64 and treat u32 as a special case. But this is certainly better than writing bits!
| } | ||
|
|
||
| #[test] | ||
| fn test_kmer_hash_independent_of_storage_width() { |
Comment on lines
+1205
to
+1208
| impl Hasher for RecordingHasher { | ||
| fn finish(&self) -> u64 { 0 } | ||
| fn write(&mut self, bytes: &[u8]) { self.0.extend_from_slice(bytes); } | ||
| } |
Owner
There was a problem hiding this comment.
this is handy for the testing, maybe we could find a way to incorporate it into the storage type more closely and design an API that make rolling hashes more natural to express while we're at it
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #15.
Fix
The
bitvecHashimpl flagged in the issue is part of it —bypassing it removes the per-bit hasher-write pattern. The remaining
platform variance comes from two paths in our own code:
self.len().hash(state)andK.hash(state)route ausizethrough
Hasher::write_usize— 4 bytes on 32-bit, 8 on 64-bit.Kmer::hashalso hashed the entire backingBitArray, sounused high bits of the storage word were included in the digest.
Using
u64::hashdirectly would still be endian-dependent (itsdefault
write_u64writes native-endian bytes), so the length goesin as
(len as u64).to_le_bytes()explicitly — fixing both thepointer-width and endianness axes.
For the bit content, a new
hash_bitshelper extracts whole bytesvia
BitField::load_le::<u8>overbs.chunks(8), batching writesthrough a 64-byte buffer.
Kmer::hashslices its bitarray toK * A::BITSfirst so storage padding can't leak in.Behavioral change: hash values for
Seq/SeqSlice/Kmerchange.The
Hashcontract (equal values → equal hashes) is preserved;existing equality-based hash tests still pass.
Tests
A small
RecordingHashercaptures every byte fed to the hasher.Two new tests:
test_hash_byte_stream_invariants: forDna(2-bit),Iupac(4-bit),
Amino(6-bit), andtext::Dna(8-bit), at lengthscovering byte-aligned, partial-trailing-byte, and long inputs —
Seq<A>and&SeqSlice<A>produce the same byte stream, andthe stream's length and prefix match the structural contract.
test_kmer_hash_independent_of_storage_width:Kmer<Dna, 10, _>with
usize/u64/u128storage all hash identically and matchthe equivalent
&SeqSlice.Out of scope
Kmer<_, _, simd::Simd<u64, 4>>(avx2) andKmer<_, _, wasm::v128>still have
todo!()Hashimpls — pre-existing, would get the sametreatment once implemented. Happy to do a follow-up.
Happy to adjust the byte layout (length-prefix size, ordering, etc.)
if you'd prefer a different convention.