Version 1.15 - #65
Merged
Merged
Conversation
…hash, and Span sections (#44) Fills in the empty Release Notes section and adds dedicated sections for accuracy/memory tuning (the b parameter), hash function selection, ConcurrentCardinalityEstimator, Merge / ParallelMerge, the CardinalityEstimatorSerializer round-trip, and the zero-allocation ICardinalityEstimatorMemory overloads. Also: - Replace the dead static.googleusercontent.com paper link with the live research.google URL. - Document the dotnet CLI install command alongside Install-Package. - Clarify the basic usage comment so the deduplication behavior of Add("Alice") twice -> Count() == 3 is explicit. Docs-only change; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ts (#45) ICardinalityEstimatorMemory: the 'with zero allocations' / 'with optimized allocation patterns' phrasing on the Span/Memory Add overloads was misleading. Memory<byte> and ReadOnlyMemory<byte> can still reference heap-allocated storage, so they are not truly zero-allocation paths. Reworded the per-overload summaries and added an interface-level remark explaining that Span<byte> / ReadOnlySpan<byte> are the actual zero-allocation entry points. CardinalityEstimatorExtensions.CreateMultiple: expanded the 'b' parameter doc to mention the default (14), the standard error formula, and the memory bounds, matching the wording on the CardinalityEstimator constructor for consistency. Docs-only change; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Validate CardinalityEstimatorSerializer input to prevent DoS The binary deserializer in CardinalityEstimatorSerializer.Read read bitsPerIndex and every length prefix (directCount, lookupSparse, lookupDense) directly from an untrusted stream and used them to allocate or loop without any bounds checks. A crafted payload could trigger huge allocations or near-infinite loops: - bitsPerIndex = 30 -> m = 2^30 -> ~1 GB lookupDense allocation - count = Int32.MaxValue for directCount -> ~2B iterations of 8-byte reads - count = Int32.MaxValue for lookupDense -> ~2 GB ReadBytes allocation - count > m for lookupSparse -> wasted reads / possible OOM Fix: validate up front and throw InvalidDataException on bad data. - bitsPerIndex must be in [4, 16] (matches CardinalityEstimator). - directCount length must be in [0, 100] (DirectCounterMaxElements). - sparse length must be in [0, m] where m = 2^bitsPerIndex. - dense length must equal m exactly. - Also detect a truncated dense payload (ReadBytes returning short). Added regression tests in CardinalityEstimatorSerializerTests covering each invalid header variant; all of them fail on the pre-fix code with either an OutOfMemoryException, an EndOfStreamException, or a wrong result, and now throw InvalidDataException as expected. Updated README and PackageReleaseNotes; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump version to 1.15.0 and scope release notes to 1.15.0 only The version-1.15 branch was never seeded with the 1.15.0 version / fresh release-notes block. Doing it now as part of this fix: - <Version>, <AssemblyVersion>, <FileVersion> -> 1.15.0(.0) - <PackageReleaseNotes> replaced with the single 1.15.0 bullet for this security fix; the stale 1.14.0 entries are dropped (they describe an already-shipped release). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Switch target frameworks from net8.0/net9.0 to net8.0/net10.0 .NET 9 is a STS release and is being dropped in favor of the new .NET 10 LTS. All three projects (library, tests, benchmarks) now multi-target net8.0;net10.0. The CI workflows install 8.0.x and 10.0.x SDKs. There is no NET9_0/NET10_0 conditional compilation in the codebase, so this is a pure TFM swap. Updated the README, ROADMAP, and a stray comment in CardinalityEstimator that mentioned '.NET 9+' for the XxHash128 default (XxHash128 actually comes from System.IO.Hashing, which works on net8.0 too, so the comment is now framework-agnostic). Added a release note bullet for 1.15.0; no version bump (the version is already 1.15.0 on this branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump System.IO.Hashing 8.0.0 -> 10.0.7 Aligns the dependency with the new net10.0 target. The package is compatible with both net8.0 and net10.0, so the net8.0 target keeps working. Bumped in both the library and the benchmark project (the benchmark would otherwise hit NU1605 from the transitively-pinned 8.0.0 reference). Updated release notes accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…entBag with ConcurrentDictionary (#48) ConcurrentCardinalityEstimator stored direct-count elements in a ConcurrentBag<ulong>, which permits duplicates. As a result, every Add, Count, Merge, and Equals had to call .Distinct().Count() on the bag - allocating a temporary collection and iterating all elements - just to get the true unique count. AddElementHashInternal performed two such Distinct() passes per call (countBefore and countAfter), turning what should be an O(1) hash-set probe into an O(n) operation that allocates twice on every add. The storage is now a ConcurrentDictionary<ulong, byte> used as a concurrent hash set: TryAdd is O(1) and returns whether the key was new, Count is O(1), and ContainsKey/Keys avoid the per-call allocations the bag forced. All call sites (Count, GetStateInternal, AddElementHashInternal, MergeInternal, EqualsInternal) and the from-state initializer were updated to match. Added 4 regression tests: high-contention duplicate adds (Count must equal distinct count exactly), correct transition past DirectCounterMaxElements, merge dedup at the direct-count layer, and a state round-trip through ToCardinalityEstimator. Release notes updated under the upcoming 1.15.0 entry; no version bump (this fix accumulates on the version-1.15 branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ensions (#49) * Close test coverage gaps across estimators, serializer, hash, and extensions Adds CoverageGapTests.cs with 47 targeted tests that exercise previously uncovered paths in CardinalityEstimator, ConcurrentCardinalityEstimator, CardinalityEstimatorSerializer, CardinalityEstimatorExtensions, Murmur3, and Fnv1A. Coverage is up from 92.06% line / 80.71% branch to 96.33% / 90.44%, with BiasCorrection, CardinalityEstimatorExtensions, CardinalityEstimatorSerializer, Fnv1A, and Murmur3 all at 100% line. Also adds the coverlet.collector dev dependency to the test project so `dotnet test --collect:""XPlat Code Coverage""` works out of the box. No production-code changes; no version bump; no release-notes entry (test-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move coverage tests into their respective per-class test files The combined CoverageGapTests.cs file deviated from the repo convention of one test class per production class. Tests have been merged into the appropriate existing test files (CardinalityEstimatorTests, CardinalityEstimatorSerializerTests, ConcurrentCardinalityEstimatorTests, CardinalityEstimatorExtensionsTests, Fnv1ATests, Murmur3Tests). No behavior change; full suite still passes 177/1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Microsoft.TestPlatform.ObjectModel 16.8.3 transitively pulled Newtonsoft.Json 9.0.1, which has the high-severity advisory GHSA-5crp-9r3c-p9vr. Pinning Newtonsoft.Json 13.0.3 directly in the test project overrides the transitive resolution and clears the warning. Test-only change; the production CardinalityEstimation package does not reference Newtonsoft.Json. No version bump, no release-notes entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add(int/uint/long/ulong/float/double) previously called BitConverter.GetBytes which heap-allocates a byte[] every call, generating GC pressure proportional to the number of inserts. They now use stackalloc with System.Buffers.Binary.BinaryPrimitives.WriteXxxLittleEndian and hand the buffer to the existing span hash delegate (hashFunctionSpan) for a true zero-allocation hot path. The byte sequence is byte-equivalent to BitConverter.GetBytes on every supported (little-endian) .NET runtime, so hash output and existing cardinality estimates are unchanged. Add(string) now encodes into a 256-byte stack buffer via Encoding.UTF8.GetBytes(string, Span<byte>) for short strings, falling back to a heap allocation only when the UTF-8 max-byte-count exceeds the threshold. Same byte sequence as before; same hashes. Applied symmetrically to CardinalityEstimator and ConcurrentCardinalityEstimator. Added byte-equivalence regression tests in both test classes. Release note added under the upcoming 1.15.0 entry; no version bump (this fix accumulates on the version-1.15 branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The HLL summation in CardinalityEstimator.Count() and ConcurrentCardinalityEstimator.ComputeCountInternal() called Math.Pow(2, -sigma) once per bucket -- up to m = 2^bitsPerIndex = 65,536 transcendental calls per Count() invocation for b=16. sigma is a bounded byte (at most bitsForHll + 1 <= 61) so 2^-sigma is exactly representable as an IEEE 754 double for every reachable value. The hot-loop call is replaced with an indexed read into a precomputed table of size 65, shared by both estimators via the new internal HllConstants helper. Results are bit-equivalent to the previous Math.Pow output, so all existing accuracy/serialization tests pass unchanged. Added regression test InversePowersOfTwo_TableMatchesMathPowExactly that pins each table entry against Math.Pow(2.0, -i) for i in [0, 64], plus InversePowersOfTwo_MaxSigmaIndexIsValidForAllBitsPerIndex which proves sigma never exceeds the table bounds across the supported bitsPerIndex range. Added a release note under the upcoming 1.15.0 entry; no version bump (this fix accumulates on the version-1.15 branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CardinalityEstimatorSerializer.Write() serialized the dense lookup array (up to 2^16 = 65,536 bytes for b=16) one byte at a time inside a foreach loop, calling BinaryWriter.Write(byte) per element. Replaced with a single BinaryWriter.Write(byte[]) bulk-copy call. On-the-wire output is byte-identical (BinaryWriter.Write(byte) and Write(byte[]) both emit raw bytes with no length prefix), so existing roundtrip and byte-layout tests cover the change: TestSerializerCardinality100000 pins the exact serialized size (21 + lookupDense.Length) and SerializerCanDeserializeVersion2Point1 reads a stored dense fixture. Added a release note under the upcoming 1.15.0 entry; no version bump (this fix accumulates on the version-1.15 branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PackageReleaseNotes (in CardinalityEstimation.csproj) and the 1.15.0 section in README.md were verbose multi-line entries. Condensed each bullet to a short phrase so the upcoming release notes scan quickly on NuGet and in the README. Docs-only change; no code paths affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ns (#55) Two tightly coupled bugs in CardinalityEstimator that masked each other: 1. The copy constructor (line 186) did not copy CountAdditions, leaving it at the default 0. A clone via 'new CardinalityEstimator(other)' silently lost the running addition count. 2. The static Merge(IEnumerable) seeded 'result' from the first non-null estimator and then unconditionally called result.Merge(estimator) on the same instance, which (with a correct copy ctor) would double-count CountAdditions for the seed element since Merge does CountAdditions += other.CountAdditions. The HLL max-register merge is idempotent so cardinality was unaffected, but the addition count was wrong. Fixed both: copy ctor now copies CountAdditions, and the seed branch in static Merge no longer re-merges the seed estimator into itself (matches the existing pattern in ConcurrentCardinalityEstimator.Merge). Added regression tests CopyConstructor_PreservesCountAdditions (fails on pre-fix code) and StaticMerge_SumsCountAdditions_DoesNotDoubleCountFirstElement (pins the invariant going forward). Added a release note under the upcoming 1.15.0 entry; no version bump (this fix accumulates on the version-1.15 branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ed delegate (#56) The (GetHashCodeSpanDelegate, CardinalityEstimatorState) constructor chained to this(state), which left both hashFunction and hashFunctionSpan null. The subsequent `if (this.hashFunction == null)` check was therefore always true, causing the constructor to overwrite both delegates with the default XxHash128 and silently discard the caller-supplied span delegate. Mirrored the correct pattern from the analogous CardinalityEstimator constructor: now checks hashFunctionSpan == null, and on the else branch wraps the provided span delegate so hashFunction routes through it. Added regression test SpanDelegateConstructor_UsesProvidedDelegateInsteadOfDefault that verifies a custom span delegate is invoked on Add (fails on pre-fix code, passes on fixed code). Release notes updated under 1.15.0; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…)/Add(byte[]) (#57) CardinalityEstimator.Add(string) and Add(byte[]) both throw ArgumentNullException(nameof(element)) for null input. The corresponding overloads on ConcurrentCardinalityEstimator did not, so passing null produced either a NullReferenceException (Add(string), via element.Length) or an ArgumentNullException with the wrong parameter name (Add(byte[]), bubbling up from the underlying hash function with paramName=source). Added explicit null guards mirroring the non-concurrent class so both APIs have the same documented contract and parameter name. Added regression tests AddString_ThrowsArgumentNullException_WhenElementIsNull and AddByteArray_ThrowsArgumentNullException_WhenElementIsNull (assert ParamName == �lement); both fail on pre-fix code. Release notes updated under 1.15.0; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#58) Every constructor of CardinalityEstimator and ConcurrentCardinalityEstimator computed the substream count m as (int)Math.Pow(2, bitsPerIndex). The deserializer (Read) already used the cheaper, exact 1 << bitsPerIndex. Switched the constructors to match: bitsPerIndex is constrained to [4, 16] so the shift is exact and avoids a transcendental floating-point call on the construction path. Pure refactor — mathematically equivalent for all valid inputs. Existing accuracy and serialization tests already exercise the resulting m. Added Constructor_LookupDenseLength_EqualsTwoToBitsPerIndex theory (b = 4, 10, 14, 16) to pin the m == 2^bitsPerIndex contract explicitly. Release notes updated under 1.15.0; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CardinalityEstimatorSerializer.Read had an empty `if (dataFormatMajorVersion >= 3) { }` branch which read as an unfinished extension point. The branch is in fact functionally required: data format v3 dropped the on-the-wire hashFunctionId byte that v2 wrote, so the v3 path correctly does nothing here while still intercepting v3+ streams to prevent them from falling through to the v2 reader (which would erroneously consume a non-existent byte).
Replaced the empty body with a comment explaining (a) why nothing is read for v3+, and (b) where the hash function ultimately comes from (caller-supplied or XxHash128 default in the CardinalityEstimator constructor).
Pure documentation change — no behavior change. Existing v3 round-trip tests already cover this code path.
Release notes updated under 1.15.0; no version bump (accumulates on version-1.15).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nts (#60) * Consolidate duplicated estimator constants and helpers into HllConstants CardinalityEstimator and ConcurrentCardinalityEstimator independently duplicated DirectCounterMaxElements, StackallocByteThreshold, GetAlphaM, GetSubAlgorithmSelectionThreshold (with identical case tables), and CreateEmptyState. HllConstants was already the home for shared HLL state (InversePowersOfTwo); moved the duplicates there as well. Both estimator classes now alias DirectCounterMaxElements and StackallocByteThreshold to HllConstants and forward to HllConstants.GetAlphaM / GetSubAlgorithmSelectionThreshold / CreateEmptyState. The aliases keep the existing call sites readable without forcing every reference to use the qualified name. Pure refactor — no behavior change. Added HllConstantsTests with theory coverage of GetAlphaM (canned + formula values), GetSubAlgorithmSelectionThreshold (Heule et al. table + out-of-range), CreateEmptyState (validation + initial state for both useDirectCount values), and pinning tests for the two constants. Existing accuracy/serialization tests continue to cover the through-the-estimator path. Release notes updated under 1.15.0; no version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Inline HllConstants references; drop estimator alias constants Per review feedback, removed the private const aliases for DirectCounterMaxElements and StackallocByteThreshold from CardinalityEstimator and ConcurrentCardinalityEstimator. All call sites and XML doc crefs now reference HllConstants.X directly, eliminating the indirection. No behavior change; release notes already cover the consolidation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iable (#61) ConcurrentCardinalityEstimator.ParallelMerge built a ParallelQuery<ConcurrentCardinalityEstimator> into a 'parallelOptions' local that was never used — the actual batch processing called batches.AsParallel() unconditionally, silently ignoring the caller's parallelismDegree argument. Removed the dead variable and applied WithDegreeOfParallelism directly to the batch query so the caller-supplied degree is honored. Release notes updated under 1.15.0; no version bump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ng (#62) Merge and Equals previously ordered the two ReaderWriterLockSlim acquisitions by comparing GetHashCode() of the two instances. Object.GetHashCode is not guaranteed unique, so two distinct instances with colliding hash codes produced an undefined ordering — opening a deadlock window when threads merged the same pair of estimators in opposite directions. Added a static Interlocked-incremented counter and a per-instance 'instanceId' long field assigned at construction. Both lock-ordering call sites now compare instanceId, which is strictly distinct between instances and therefore deadlock-free. Added two regression tests: one pinning instanceId uniqueness across 1000 allocations, and a cross-pair high-concurrency Merge stress test that bounds the run to 30 seconds to surface any deadlock as a failure. Release notes updated under 1.15.0; no version bump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…versions (#63) ConcurrentCardinalityEstimator(CardinalityEstimator) silently substituted the default XxHash128 because CardinalityEstimator exposed no API to read its hash function back. Symmetrically, ConcurrentCardinalityEstimator.ToCardinalityEstimator() forwarded only the byte-array delegate, causing the target to synthesise a span delegate as (x) => hashFunction(x.ToArray()) and lose the optimized zero-allocation span path. Added public HashFunction and HashFunctionSpan get-only properties on both CardinalityEstimator and ConcurrentCardinalityEstimator, plus a new internal CardinalityEstimator(GetHashCodeDelegate, GetHashCodeSpanDelegate, CardinalityEstimatorState) constructor that preserves both delegates verbatim. The CE -> CCE conversion ctor and CCE.ToCardinalityEstimator() now use these to keep both hash paths intact, making the conversions truly lossless. Three regression tests cover (a) CE -> CCE preserving a custom Fnv1A delegate by reference, (b) CCE -> CE preserving both delegates by reference, and (c) the new properties returning the supplied delegate. Release notes updated under 1.15.0; no version bump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test project referenced coverlet.collector 10.0.0 but no workflow or script ever invoked --collect:"XPlat Code Coverage", so the package was dead weight. Replaced it with a dotnet-coverage local tool manifest at .config/dotnet-tools.json so developers can opt into coverage when they want it without dragging an unused package into every restore. Workflow: dotnet tool restore dotnet build dotnet dotnet-coverage collect --output-format cobertura --output coverage.xml "dotnet test --no-build --nologo" Smoke-tested locally: produces a Cobertura XML reporting 98% line / 91% branch coverage across both target frameworks. CI workflows are unchanged (per the chosen scope: developer-local only). Release notes updated under 1.15.0; no version bump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 5 second Task.WaitAll budget was too tight for shared CI runners. Locally the merges complete in tens of milliseconds, but on a cold ubuntu-latest runner under contention this test recently timed out exactly at the 5 s mark on net8.0 (PR #65 / run 25215421113), reporting 'Merges should complete without deadlock' even though no deadlock existed. Bumped the budget to 30 s, matching the deadlock-detection budget used by Merge_CrossPairs_HighConcurrency_DoesNotDeadlock. This still fails fast on a real deadlock while eliminating the flake. Test-only change; no production code touched, so no PackageReleaseNotes entry. No version bump (accumulates on version-1.15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
No description provided.