refactor(hashing): name the algorithm in the unsigned hashers - #355
Conversation
The bare `UIntNN Hasher` name meant opposite tiers of the escalation ladder in the two unsigned widths: `UInt32Hasher` was the cheap XOR-fold, `UInt64Hasher` the strong Murmur3 fmix64 finalizer. Hasher choice is the main knob this library exposes and callers pick by analogy across widths, so moving from uint to ulong keys silently changed hash strength, not just key width. The signed families never had the problem because they name the algorithm in the type. `UInt32WangNaiveHasher` and `UInt64Murmur3Hasher` now ship. The old names remain as [Obsolete] aliases that forward to them rather than repeating the mixer, so the pairs cannot drift and no hash value moved. `UInt64Hasher` keeps its IHashProvider64<ulong> implementation — dropping it would push an existing sketch back to the 2^32 entropy floor as a side effect of a naming change. `IntegerHasherFamilyNamingTests` is the regression guard: it fails on a new bare-named integer hasher or a width missing a tier, records the signed-only identity tier as deliberate, and pins the one unsigned/signed pair that is not bit-identical (the 32-bit naive fold, arithmetic shift vs logical). Closes #297. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage
|
There was a problem hiding this comment.
Pull request overview
Renames unsigned integer hashers to expose their algorithms while preserving compatibility through obsolete aliases.
Changes:
- Adds
UInt32WangNaiveHasherandUInt64Murmur3Hasher. - Adds compatibility, contract, AOT, and parity tests.
- Updates benchmarks, documentation, migration guidance, and release notes.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Celerity/TypeForwarders.cs |
Forwards new hasher types and aliases. |
src/Celerity.Tests/Hashing/UInt64Murmur3HasherTests.cs |
Tests the renamed 64-bit hasher. |
src/Celerity.Tests/Hashing/UInt64HasherTests.cs |
Tests deprecated alias compatibility. |
src/Celerity.Tests/Hashing/UInt32WangNaiveHasherTests.cs |
Tests the renamed 32-bit hasher. |
src/Celerity.Tests/Hashing/UInt32HasherTests.cs |
Tests deprecated alias compatibility. |
src/Celerity.Tests/Hashing/IntegerHasherFamilyNamingTests.cs |
Guards family naming and parity. |
src/Celerity.Tests/Hashing/HashProvider64ContractTests.cs |
Extends 64-bit provider contracts. |
src/Celerity.Hashing/UInt64WangNaiveHasher.cs |
Updates cross-references. |
src/Celerity.Hashing/UInt64WangHasher.cs |
Updates cross-references. |
src/Celerity.Hashing/UInt64Murmur3Hasher.cs |
Adds the named Murmur3 implementation. |
src/Celerity.Hashing/UInt64Hasher.cs |
Converts the old type into an alias. |
src/Celerity.Hashing/UInt32WangNaiveHasher.cs |
Adds the named XOR-fold implementation. |
src/Celerity.Hashing/UInt32WangHasher.cs |
Updates cross-references. |
src/Celerity.Hashing/UInt32Murmur3Hasher.cs |
Updates cross-references. |
src/Celerity.Hashing/UInt32Hasher.cs |
Converts the old type into an alias. |
src/Celerity.Hashing/README.md |
Documents explicit hasher naming. |
src/Celerity.Benchmarks/IntegerHasherBenchmark.cs |
Renames benchmark series. |
src/Celerity.Benchmarks/HashQualityReportRunner.cs |
Updates and reorders report rows. |
src/Celerity.AotSmokeTest/Program.cs |
Exercises new names and aliases under AOT. |
ROADMAP.md |
Records the completed naming decision. |
README.md |
Updates hasher selection guidance. |
docs/README.md |
Updates the hashing API index. |
docs/migration.md |
Adds migration instructions. |
docs/api/hashing.md |
Documents new types and aliases. |
docs/aot.md |
Updates AOT coverage documentation. |
CHANGELOG.md |
Records additions and deprecations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Four fixes from the first review round: - The "Deprecated hasher aliases" heading was inserted mid-section, stranding DefaultHasher<T>'s closing paragraph under it so its "It" referred to the wrong subject. Moved below that paragraph. - The two new hashers' remarks promised removal "for one release" while the Obsolete message and the migration guide say "a future major version". Aligned on the latter, in the remarks and in ROADMAP.md. - Renamed the new suites' methods to Method_ShouldExpectedBehavior_WhenCondition. They had mirrored the legacy names of the files they replaced; mirroring the coverage is the point, not mirroring stale names. - Condensed the CHANGELOG entry. The section becomes the release body verbatim and this repo has already hit GitHub's size limit once, so the test and doc rollout bullets fold into the API bullet, which now points at the migration guide instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Celerity.Tests/Hashing/IntegerHasherFamilyNamingTests.cs:74
- This “adds nothing” premise does not hold for the library's primary generic use case.
CelerityDictionary<TKey, ..., THasher>requiresTHasher : IHashProvider<TKey>(CelerityDictionary.cs:19-21), soInt32IdentityHashercannot be supplied for auintcollection and there is no caller-side cast to insert—the collection invokesHashinternally. The negative assertions therefore freeze out a useful zero-workIHashProvider<uint>/IHashProvider<ulong>API for an unrelated naming regression. Remove this absence contract (and the matching ROADMAP claim); the test should only pin the three algorithm-named mixing tiers relevant to this PR.
// The zero-work floor is reached from an unsigned key with a free cast at the call
// site (`new Int32IdentityHasher().Hash((int)u)`), so a UInt32IdentityHasher would be
// a type that adds nothing. Recorded here rather than left to be rediscovered as a gap.
ROADMAP.md:244
- The claim that an unsigned identity hasher “adds nothing” is inaccurate for collection consumers. The collections constrain
THashertoIHashProvider<TKey>, soInt32IdentityHasher : IHashProvider<int>cannot hash aCelerityDictionary<uint, ...>; users cannot insert the suggested cast because hashing occurs inside the collection. Remove this design conclusion (and its new negative test) or document a rationale that accounts for generic collection usage.
- The bare `UIntNN Hasher` name meant opposite tiers of the escalation ladder in the two unsigned widths: `UInt32Hasher` was the cheap XOR-fold while `UInt64Hasher` was the strong Murmur3 `fmix64` finalizer, so a caller who benchmarked on `uint` and then moved to `ulong` keys by analogy silently changed hash *strength*, not just key width. Hasher selection is the main knob this library exposes and the signed families never had the problem, because they name the algorithm in the type. Status: `done` — option 1 of the issue (rename for explicitness, old names kept as `[Obsolete]` aliases until a future major version): `UInt32WangNaiveHasher` and `UInt64Murmur3Hasher` ship, and the aliases forward to them rather than repeating the mixer, so the pairs cannot drift and no hash value moved. Two calls are worth recording. First, `UInt64Hasher` keeps its `IHashProvider64<ulong>` implementation rather than being reduced to the 32-bit surface: dropping it would push an existing sketch back to the 2^32 entropy floor `IHashProvider64<T>` exists to escape, silently, as a side effect of a *naming* change. Second, the identity tier stays signed-only — an unsigned key reaches the zero-work floor with a free cast at the call site, so a `UInt32IdentityHasher` would be a type that adds nothing — and that asymmetry is now pinned by a test rather than left to be rediscovered as a gap. The regression guard is the family-wide `IntegerHasherFamilyNamingTests`, which fails on a new bare-named integer hasher or a width missing a tier; it also pins the one unsigned/signed pair that is deliberately *not* bit-identical, the 32-bit naive fold, whose shift is arithmetic on `int` and logical on `uint`. Tracked in [#297](https://github.com/marius-bughiu/Celerity/issues/297).
The naming test asserted that UInt32IdentityHasher / UInt64IdentityHasher must not exist, on the premise that an unsigned caller reaches the zero-work floor with a cast. That premise only holds for a direct Hash call. The collections take the hasher as a type parameter constrained to IHashProvider<TKey> and invoke it internally, so a uint-keyed collection cannot be given a signed identity hasher and the caller has nowhere to put a cast — meaning half the integer widths have no reachable zero-work tier at all. Removed the assertion and the matching ROADMAP claim. The test now pins only the three algorithm-named mixing tiers, which is what this change is about. The real gap is filed as #357 rather than settled inside a rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 2 — both findings were in the suppressed block, and both were right.
The premise I wrote — that an unsigned caller reaches the zero-work floor with Removed the whole I did not add the two hashers here. Closing the identity tier is a separate call — two-line types are close to the "yet another hasher variant" shape this project resists, and the argument for them (completing an existing ladder against a documented decision rule, currently unreachable from the collections) deserves triage rather than being smuggled into a rename. Filed as #357 with that tension stated explicitly, and the ROADMAP entry points at it. Local re-verification: build clean, 1224 hashing tests pass on each of net8.0 / net9.0 / net10.0, doc-anchor check green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ROADMAP.md:244
- The PR description still states that the unsigned identity gap is deliberate and pinned by a test, and that these aliases remain for only a release. This updated roadmap records the opposite: identity is an open gap tracked by #357, no test pins the asymmetry, and removal is deferred to a future major version. Update the PR's “What ships,” cross-cutting-test, and roadmap text so reviewers are not approving stale design and lifecycle claims.
- The bare `UIntNN Hasher` name meant opposite tiers of the escalation ladder in the two unsigned widths: `UInt32Hasher` was the cheap XOR-fold while `UInt64Hasher` was the strong Murmur3 `fmix64` finalizer, so a caller who benchmarked on `uint` and then moved to `ulong` keys by analogy silently changed hash *strength*, not just key width. Hasher selection is the main knob this library exposes and the signed families never had the problem, because they name the algorithm in the type. Status: `done` — option 1 of the issue (rename for explicitness, old names kept as `[Obsolete]` aliases until a future major version): `UInt32WangNaiveHasher` and `UInt64Murmur3Hasher` ship, and the aliases forward to them rather than repeating the mixer, so the pairs cannot drift and no hash value moved. Two calls are worth recording. First, `UInt64Hasher` keeps its `IHashProvider64<ulong>` implementation rather than being reduced to the 32-bit surface: dropping it would push an existing sketch back to the 2^32 entropy floor `IHashProvider64<T>` exists to escape, silently, as a side effect of a *naming* change. Second, the identity tier is still signed-only, and that is now recorded as an open gap rather than a decision: the first draft argued an unsigned key reaches the zero-work floor with a cast at the call site, which is false for the primary use case — the collections constrain `THasher` to `IHashProvider<TKey>` and invoke `Hash` internally, so no `IHashProvider<uint>` identity hasher means no zero-work floor for a `uint`-keyed collection at all. Filed as [#357](https://github.com/marius-bughiu/Celerity/issues/357) rather than settled inside a naming change. The regression guard is the family-wide `IntegerHasherFamilyNamingTests`, which fails on a new bare-named integer hasher or a width missing a tier; it also pins the one unsigned/signed pair that is deliberately *not* bit-identical, the 32-bit naive fold, whose shift is arithmetic on `int` and logical on `uint`. Tracked in [#297](https://github.com/marius-bughiu/Celerity/issues/297).
docs/api/hashing.md:81
- This roster omits
UInt64Hasher, even though the alias still implementsIHashProvider64<ulong>and the contract test intentionally keeps both types. Because the table claims to list hashers withHash64, it now contradicts the API and the compatibility note at line 766. Keep the alias in the row, marked obsolete.
| `ulong` | `UInt64WangHasher`, `UInt64Murmur3Hasher` | `Hash` is the low 32 bits of `Hash64` |
The "which built-in hashers implement it" table lost UInt64Hasher when the row was updated to name UInt64Murmur3Hasher. The alias still implements IHashProvider64<ulong> — deliberately, so a sketch parameterized on it does not drop to the 2^32 floor mid-migration — and the contract test roster still carries it, so the table was the one surface saying otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 3 — two suppressed findings again, both correct.
I could not find a stale "for one release" claim in the description — that phrase was in the two hashers' XML remarks and in No source change in this round beyond the one table cell; doc-anchor check green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
README.md:675
- This 64-bit hasher roster is now incomplete:
UInt64Hasherstill implementsIHashProvider64<ulong>for compatibility (and is retained in the exact roster inHashProvider64ContractTestsanddocs/api/hashing.md), but replacing it here removes it entirely. Keep the obsolete alias listed so the README does not contradict the compatibility contract.
> **Counting past ~10^8 distinct elements? Pick a 64-bit hasher.** The probabilistic sketches (`HyperLogLog`, `BloomFilter`, `CuckooFilter`, `XorFilter`, `CountMinSketch`) never store the element, so two elements that hash alike are indistinguishable forever — and a 32-bit hash reaches only 2^32 ≈ 4.3 billion values, no matter how it is widened. Hashers that carry genuine 64-bit entropy implement **`IHashProvider64<T>`** and the sketches route through it automatically: `Int64WangHasher` / `Int64Murmur3Hasher` (`long`), `UInt64WangHasher` / `UInt64Murmur3Hasher` (`ulong`), `GuidHasher` (`Guid`), and the nine 64-bit `string` hashers (`StringXxHash64Hasher`, `StringXxHash3Hasher`, `StringCityHash64Hasher`, `StringMetroHash64Hasher`, `StringHighwayHash64Hasher`, `StringSipHash13Hasher`, `StringSipHash24Hasher`, `StringFnV1A64Hasher`, `StringFnV164Hasher`). They already computed 64 bits internally and folded them away, so `Hash64` costs nothing extra. With a 32-bit-only hasher the sketches still work — `HyperLogLog` applies the classical large-range correction so its estimate stays honest — but the entropy floor is real. See [`IHashProvider64<T>`](docs/api/hashing.md#ihashprovider64t).
Same omission as the API-reference roster: replacing UInt64Hasher with UInt64Murmur3Hasher in the sketch-entropy note dropped a type that still implements IHashProvider64<ulong>. Listed again, marked deprecated, so the sentence is complete without pointing a new reader at the old name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 4 — one suppressed finding, the same omission as round 3 in a second place. Agreed, fixed in 98a5f13.
One deliberate difference from the reference table, worth stating rather than leaving to be inferred. That table is a roster — it is headed "Which built-in hashers implement it" — so the alias belongs in it plainly. The README sentence is guidance: it tells someone counting past 10^8 elements which hasher to reach for. Listing a deprecated type there unqualified would hand a new reader the name we are trying to retire. So it now reads Doc-anchor check green. No source change. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Celerity.Tests/Hashing/IntegerHasherFamilyNamingTests.cs:12
- The summary says every unsigned hasher agrees bit-for-bit with its signed peer, but this suite explicitly documents and tests the
UInt32WangNaiveHasherexception below. Reword this sentence so the class contract does not contradict its own regression test.
/// mixing tiers, and an unsigned hasher agrees bit-for-bit with its signed peer.
The class summary claimed every unsigned hasher agrees bit-for-bit with its signed peer, while a test three methods down pins the one pair that does not. Named the exception in the summary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 5 — one suppressed finding, correct. Fixed in 1a63a55.
|
|
Note on the benchmark comment this run will produce, so three rows do not read as an anomaly. Renaming the arms means The mirror image is that the old Nothing here needs action — flagging it because a reviewer scanning the comment would reasonably wonder why three |
Benchmarks3 regressions Highlights
Collections (646)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( Observed spread of this run — |Δ| across all 754 paired rows: p50 0.5%, p90 6.6%, p95 11.2% (nearest-rank). A pull request usually touches a handful of these, so this is mostly the runner's own drift between the two slices — but a change to a shared primitive moves many rows at once and would raise these figures itself, so read it as the run's spread rather than as a floor the PR cannot have caused. A flag that does not stand out against it is worth re-running before acting on. |
|
CI is green — all 21 checks pass. Two things from the benchmark comment worth reading before the The rename behaved as predicted. The three arms render as
The 3 flagged regressions are not this PR. I checked rather than asserting it:
The runs own spread agrees: p50 0.5%, p90 6.6%, p95 11.2% across 754 paired rows. Two of the three flags sit at or near p95, and Nothing outstanding from my side: six Copilot rounds, all findings addressed, the last two clean. |
Closes #297.
The defect
The bare
UIntNN Hashername mapped to opposite tiers of the escalation ladder in the two unsigned widths:UInt32Hasherkey ^ (key >> 16)Int32WangNaiveHasherUInt64Hasherfmix64finalizerInt64Murmur3HasherHasher selection is the main knob this library exposes and callers pick by analogy across key widths — that is how the rest of the surface is designed to be read. Someone who benchmarked on
uintwithUInt32Hasherand then moved toulongkeys withUInt64Hashersilently changed hash strength, not just key width, and nothing in the name said so. The signed families never had the problem because they name the algorithm in the type.The package README already described the scheme this PR makes true: "
*WangNaiveHasher,*WangHasher,*Murmur3Hasheracrossint/long/uint/ulong" — a claim that was false for two of the twelve slots.What ships
Option 1 of the issue — rename for explicitness, old names kept as
[Obsolete]aliases for a release.UInt32WangNaiveHasherandUInt64Murmur3Hasherare the new names. Same hash values.UInt32Hasher/UInt64Hasherare[Obsolete](warning, not error) and forward to the new types rather than repeating the mixer, so the two cannot drift.UInt64HasherkeepsIHashProvider64<ulong>. Dropping it would have pushed an existing sketch back to the 2^32 entropy floorIHashProvider64<T>exists to escape — silently, as a side effect of a naming change.THashertoIHashProvider<TKey>and invokeHashinternally, soInt32IdentityHashercannot serve aCelerityDictionary<uint, …>and there is no call site to cast in —uint/ulong-keyed collections have no reachable zero-work tier at all. The claim and its test were removed; the gap is filed as No identity hasher for uint / ulong — the zero-work tier is unreachable from a hasher-parameterized collection #357 rather than settled inside a rename.Compatibility: additive and source-compatible. Existing code compiles with a
CS0618warning pointing at the replacement; the type forwarders for the aliases are unchanged and two new ones were added, so binary compatibility is preserved on both.EnablePackageValidationruns against the 2.5.0 baseline on everypack.Parity rollout
Collection / API —
src/Celerity.Hashing/UInt32WangNaiveHasher.cs,UInt64Murmur3Hasher.cs(new);UInt32Hasher.cs,UInt64Hasher.cs(now aliases); cross-reference fixes inUInt32WangHasher.cs,UInt32Murmur3Hasher.cs,UInt64WangHasher.cs,UInt64WangNaiveHasher.cs; two new entries insrc/Celerity/TypeForwarders.cs.Dedicated tests —
UInt32WangNaiveHasherTests/UInt64Murmur3HasherTestsmirror the suites the old names carried.UInt32HasherTests/UInt64HasherTestsare now alias tests: the original anchor vectors (so the alias's behaviour is pinned, not just its delegation), agreement with the replacement across a key sweep, the[Obsolete]attribute, and — forUInt64Hasher— that it still implementsIHashProvider64<ulong>.Cross-cutting tests —
HashProvider64ContractTests(the family roster) gainsUInt64Murmur3Hasherand keepsUInt64Hasher, with reduction and signed-peer agreement rows for both. NewIntegerHasherFamilyNamingTestsis the regression guard for the drift itself: every width ships the same three named mixing tiers, no live integer hasher carries a bare width-only name, and the unsigned/signed bit-pattern agreement is pinned — including the one pair that deliberately diverges, the 32-bit naive fold, whose>> 16is arithmetic onintand logical onuint(-1folds to0;0xFFFFFFFFfolds to0xFFFF0000). It deliberately says nothing about the identity tier (see above).Benchmarks — no new class: this is a rename, not a new algorithm, and
IntegerHasherBenchmarkis already registered inProgram.cs. Its arms were renamed with the types —UInt32_Default→UInt32_WangNaive,UInt64_Default→UInt64_Murmur3,UInt64_Default_Hash64→UInt64_Murmur3_Hash64— which is the point: the dashboard now labels auint/ulongbar with its algorithm exactly like theint/longbars. The renamed rows start a new series on the tracked chart; the old_Defaultseries stops at this commit.HashQualityReportRunnerfollows, and itsulongblock is reordered cheapest-first to match the other three widths.Dashboard — no
COLLECTIONSentry applies (that array is collections-only; hashers are regex-parsed out of the benchmark names byparseHasher, and the shape{Type}_{Hasher}is unchanged).scripts/check_dashboard_coverage.jsandscripts/check_doc_anchors.jsboth pass locally.web/index.htmlnames no individual hasher, so no ship card changes.Docs —
docs/api/hashing.md: full sections for both new types (algorithm, zero-mapping note, the arithmetic-vs-logical-shift caveat), theIHashProvider64roster table, the "choosing a hasher" table, the measured-distribution table (reordered to the ladder), and a new Deprecated hasher aliases section with a migration snippet.docs/migration.md: a rename section that also flags the substantive question — code that picked both by analogy may not have been asking for what it thought. PlusREADME.md,docs/README.md,docs/aot.md, andsrc/Celerity.Hashing/README.md.CHANGELOG —
[Unreleased]gains one### Addedbullet (the types, pointing at the migration guide), one### Changedbullet (the benchmark arm rename, which starts a new dashboard series) and a new### Deprecatedsection. Condensed on review: the section is lifted verbatim as the release body and this repo has already had a release half-fail on an over-long one, so the rollout detail lives here instead.ROADMAP — a
doneentry under 2.4.0's "Identified after the review" group, recording theIHashProvider64call and the open identity gap.Review rounds
Three Copilot rounds; every finding was acted on. Round 1 (6 comments) caught a heading inserted mid-section that stranded
DefaultHasher<T>'s closing paragraph, an "for one release" lifecycle phrase contradicting the[Obsolete]message, legacy test-method names in the new suites, and CHANGELOG verbosity. Round 2 and 3 had no inline comments but real findings in the suppressed block: the false unsigned-identity premise above, and anIHashProvider64roster table that had dropped the still-implementingUInt64Hasheralias. Fixed in3f3ac5a,7bbc7c3and2730330.Test plan
dotnet build— clean, 0 warnings.dotnet test— 5650 passed, 0 failed on each of net8.0 / net9.0 / net10.0, plus the three showcase test projects.coverage.runsettings, net10.0):Celerity.Hashingat 100% line / 100% branch. The obsolete aliases are excluded automatically by coverlet's defaultExcludeByAttribute, and are covered by tests regardless.CelerityDictionaryand asserts each alias agrees with its replacement, so ILC still compiles the alias instantiations.node scripts/check_doc_anchors.jsand--self-test— 574 links resolve.node scripts/check_dashboard_coverage.js— 155 cards across 47 collections.ubuntu/windows/macos×net8.0/net9.0/net10.0), the coverage gate,aot-publish,doc-anchors, andpackwithEnablePackageValidationagainst the 2.5.0 baseline.main— the hasher section should showWangNaive/Murmur3labels on theuintandulonggroups where it previously showedDefault.🤖 Generated with Claude Code