Skip to content

Commit 7bbc7c3

Browse files
marius-bughiuclaude
andcommitted
fix(hashing): stop freezing out an unsigned identity hasher
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>
1 parent 3f3ac5a commit 7bbc7c3

2 files changed

Lines changed: 1 addition & 13 deletions

File tree

ROADMAP.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ The 2026-Q3 review surveyed the shipped surface against the current .NET 8/9/10
241241

242242
- `SegmentTree<T, TMonoid>` — range aggregates over an arbitrary associative fold. The gap was written down in the library's own documentation: the `FenwickTree<T>` section of the API reference closed by saying a segment tree "are the next step (not shipped)". Fenwick is constrained to `INumber<T>` for a structural reason, not a stylistic one — its range query is the *difference* of two prefix folds, so the operation must have an inverse — which left the entire non-invertible half of the range-query space (min, max, gcd, bitwise and/or, any user-written fold) unreachable, with no BCL counterpart either. Status: `done` — `IMonoid<T>` ships as a `struct` type parameter alongside five built-in folds, so `Combine` inlines rather than costing a virtual call per level. Three calls are worth recording. First, the layout is the flat **`2n`** array, not the power-of-two-padded `4n` one that is usually recommended: the objection to `2n` is that the leaves sit in a rotated order at non-power-of-two lengths, so an internal node can span a wrapped range — but a query that walks outward from both ends into two separate accumulators never combines such a node into the wrong side, and an exhaustive differential sweep over every length and every range under a **non-commutative** fold pins that. A commutative fold cannot observe the difference, which is why min/max/sum could not be the oracle and the fuzz target and the differential suite both run "first non-zero wins" and string concatenation instead. The one visible consequence is that `Aggregate` is a query rather than a root read. Second, **lazy propagation was left out** rather than half-shipped: range updates need a second monoid describing how updates compose plus a distributive law relating the two, which is a different type with a different contract, and it is stated as an exclusion on every doc surface. Third, `T` is deliberately **unconstrained** — a `string`-concatenation monoid is a legitimate fold and the tree's own storage does not care — where the sibling `FenwickTree<T>` is `struct, INumber<T>`. The kill criterion (≥10x over the array scan on interleaved update + range-min at 100k) was measured after implementation and cleared at **14.8x**, with **81x** on a query batch against a pre-built tree; at 1k it is only 1.4x, and the README and API reference both lead with that rather than quoting the headline alone. The floating-point caveat on `MinMonoid` / `MaxMonoid` (the identity is the largest / smallest *finite* value, and a `NaN` resolves by operand position) is documented on the type, in the API reference and in the tests. Tracked in [#348](https://github.com/marius-bughiu/Celerity/issues/348).
243243

244-
- 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).
244+
- 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).
245245

246246
Two areas were judged real but deliberately deferred rather than rostered: a `Celerity.Statistics` package (DDSketch / reservoir sampling / running moments — a coherent fourth axis, but two new packages in one cycle is too much at once), and a batch of fuzz-target and AOT-smoke-coverage gaps (real, but low expected defect yield; better folded into whichever collection PR lands next than pursued on their own).
247247

src/Celerity.Tests/Hashing/IntegerHasherFamilyNamingTests.cs

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,6 @@ public void NoLiveIntegerHasher_ShouldCarryABareWidthOnlyName()
6666
Assert.Empty(offenders);
6767
}
6868

69-
[Fact]
70-
public void TheIdentityTier_ShouldRemainSignedOnly_AsADeliberateAsymmetry()
71-
{
72-
// The zero-work floor is reached from an unsigned key with a free cast at the call
73-
// site (`new Int32IdentityHasher().Hash((int)u)`), so a UInt32IdentityHasher would be
74-
// a type that adds nothing. Recorded here rather than left to be rediscovered as a gap.
75-
Assert.Contains(IntegerHasherTypes(), t => t.Name == "Int32IdentityHasher");
76-
Assert.Contains(IntegerHasherTypes(), t => t.Name == "Int64IdentityHasher");
77-
Assert.DoesNotContain(IntegerHasherTypes(), t => t.Name == "UInt32IdentityHasher");
78-
Assert.DoesNotContain(IntegerHasherTypes(), t => t.Name == "UInt64IdentityHasher");
79-
}
80-
8169
// ── Cross-width agreement on the same bit pattern ─────────────────────────
8270

8371
public static TheoryData<int> Int32Keys =>

0 commit comments

Comments
 (0)