refactor: harden public APIs and library usability - #247
Merged
tisonkun merged 37 commits intoAug 30, 2026
Conversation
tisonkun
changed the title
The previous test processed ten million distinct values only to exercise a CPC union after it entered Sliding flavor. Its name and assertions did not state that boundary, so the workload looked arbitrary and dominated the CPC integration-test cost. Use 32 batches of 8,192 values instead and assert the Sliding coupon threshold explicitly. The test still compares the union result with a single sketch over the same stream, while reducing the update count by about 38 times.
An empty split-point slice is a valid CDF or PMF request: zero split points define one bin containing the full distribution. The validator instead formed `0..len - 1`, which underflowed before the query could return that bin. Validate ordering with `windows(2)` and check NaNs independently. Mutable and frozen digests now both return `[1.0]` for empty CDF and PMF split points, with a regression covering all four calls.
Bloom filters assembled at runtime can legitimately differ in capacity, hash count, or seed. Treating that input mismatch as an assertion made union and intersection unsafe to use at service and storage boundaries. Return `InvalidArgument` before mutating the destination bit array, and return `Ok(())` after a compatible operation. Callers must now handle the `Result`; the integration tests cover both successful set operations and incompatible seeds.
Count-Min sketches from different producers may have different hash counts, bucket counts, or seeds. Those are recoverable compatibility errors, not internal invariants that should panic a process. Validate the complete public configuration before changing counters and return `InvalidArgument` on a mismatch. This also removes the self-pointer assertion, which safe Rust cannot reach through simultaneous `&mut self` and `&Self` borrows. Merge examples and tests now handle the `Result`.
A seed mismatch is possible when a union consumes sketches loaded from independent stores or processes. The previous assertion turned that ordinary composition error into a panic. Check the seed before any union state transition, return `InvalidArgument` with the expected and actual seeds, and make every successful flavor path return `Ok(())`. Tests verify the mismatch and update all successful callers to handle the new result.
The public sizing helpers accepted values such as zero items, NaN probabilities, and unsupported bit counts. Floating-point casts and clamping then produced plausible-looking configurations, while the accuracy builder duplicated only part of the validation. Make all three helpers return `Result`, reject invalid domains with `InvalidArgument`, and have `build` delegate to those helpers so direct and builder-based sizing share one contract. The migration is to propagate or unwrap the returned result.
`max_serialized_bytes` is a public planning helper whose `lg_k` normally comes from caller configuration. Panicking for an out-of-range value made capacity planning less robust than constructing a CPC sketch with the same input. Return `InvalidArgument` outside the supported `[4, 26]` range and preserve the existing size calculation for valid values. Boundary tests cover both sides of the range and a normal configuration.
`FrequentItemsSketch::new` accepted power-of-two map sizes below eight and silently promoted them inside the private constructor. A caller asking for one, two, or four entries therefore received a differently configured sketch without an error. Reject every map size below the algorithm minimum at the public boundary. One table-driven regression replaces the duplicate item-type tests because map-size validation is independent of the stored key type.
The constructor accepts an actual maximum map size, but the public error helpers accepted its base-two logarithm. That unit mismatch was easy to misuse, exposed an implementation detail, and allowed unchecked shift inputs; `apriori_error` also used a signed weight although stream weights are unsigned. Replace `epsilon_for_lg` with fallible `epsilon_for_max_map_size`, make `apriori_error` accept the same map-size unit and a `u64` weight, and expose `max_map_size` for round-tripping configuration. Constructor and helpers now share one validator, with valid and invalid examples covered.
A call such as `deserialize(bytes, false)` does not reveal which wire format is being selected. The DataSketches C++ float image does not encode its scalar width, so automatic detection cannot remove that choice, but a boolean makes it too easy to reverse. Make the standard double-precision path `deserialize(bytes)` and add the explicit `deserialize_f32(bytes)` entry point for C++ `tdigest<float>` images. Both delegate to one private decoder; reference-format auto-detection remains unchanged, and fixtures plus benchmarks exercise the named paths.
The immutable `TDigest` is the natural query and sharing form, but persistence required callers to convert through `TDigestMut`. That added ownership churn and made the two public forms unnecessarily asymmetric. Extract the existing compressed-image writer into a shared helper, then add `serialize`, `deserialize`, and `deserialize_f32` to `TDigest`. Mutable serialization still compresses first, immutable deserialization still passes through the validated mutable decoder, and the existing byte-stability snapshots ensure the wire format does not change.
`CpcUnion::num_coupons` exposed whether the operator currently stored an accumulator sketch or a bit matrix solely so one integration test could compare internal counts. Keeping that hook made the union representation part of the public compatibility surface. Remove the hook and assert behavior through `to_sketch`: the regression proves it reaches Sliding flavor and that its estimate agrees with a sketch over the same stream. Callers needing CPC diagnostics can inspect the resulting `CpcSketch`.
`#[doc(hidden)]` does not make a `pub` method private or exempt it from semantic-versioning commitments. These four REQ methods exposed compactor levels, nominal capacities, retained counts, and a recomputed weight without a supported caller contract, and no production or integration code used them. Remove the unused diagnostics before release so compactor layout can evolve without preserving test scaffolding as public API. Supported queries, iteration, serialization, and observable sketch state are unchanged.
Theta and Tuple Jaccard operators returned a type that callers had to import from the implementation-oriented `thetacommon` module. That leaked the internal sharing arrangement and made each sketch family appear incomplete. Re-export the same `JaccardSimilarity` type from both `theta` and `tuple`, and make the common module crate-private. Only the import path changes; the result representation and operator behavior remain identical.
tisonkun
force-pushed
the
codex/library-usability-hardening
branch
from
August 30, 2026 13:37
edc946a to
c1ba437
Compare
`TupleEntry` was externally reachable only through the re-export in the public `tuple` module. Its defining `hash_table` module is private, so the module boundary already keeps its unrestricted `pub` items internal once that re-export is removed. Remove the re-export without restating the enclosing visibility restriction on the entry, its accessors, or the table alias. Update the compact-sketch documentation and changelog to describe the remaining public iterator contract.
Windowed and batched users need to reuse a Count-Min configuration across independent streams. Reconstructing the sketch for every window reallocates the entire counter table even though its dimensions and seed do not change. Add `reset` to zero the existing table and total weight while preserving hashes, buckets, seed, and allocation. The regression verifies the cleared observations, unchanged configuration, and unchanged estimated allocation size.
Repeated aggregation windows otherwise have to discard a mutable T-Digest and its centroid capacity. Reusing that storage avoids allocation churn, but every piece of distribution state must return to the constructor invariant. Add `reset` to clear centroids and the unmerged tail while restoring extrema, weight, and merge direction; `k` and vector capacity are retained. The regression checks empty queries and metadata, stable estimated allocation size, and a successful update after reuse.
The crate enables no sketch features by default, yet its crate-level documentation did not show how to enable one or distinguish overlapping algorithm families. The compatibility text also risked implying that a portable serialized format made ordinary Rust `Hash` updates portable across languages. Document feature activation, sketch selection by workload, and the exact `hash::value` wrappers needed for strings, floats, and short integers. Also call out the empty-string behavior used by other DataSketches implementations so cross-language unions do not silently represent different inputs.
Count-Min supports narrow integer counters, but ordinary addition panicked on overflow in debug builds and wrapped in release builds. Taking the absolute value of the minimum signed weight had the same debug overflow, making results depend on the build profile. Add private saturating absolute-value and addition operations for every supported counter type, and use them for updates, total weight, merges, and upper bounds. Regressions cover unsigned update and merge saturation plus the minimum signed weight.
Frequent-items used unchecked `u64` addition in tracked counters, stream weight, purge offsets, merges, and reported upper bounds. Large weighted updates therefore panicked in debug builds but wrapped to small values in release builds, invalidating frequency ordering and error reports. Saturate every count path at `u64::MAX` and remove the impossible positive-count assertion after the zero check. The regression creates a nonzero purge offset, applies a maximum weight through owned and borrowed updates, merges another sketch, and verifies all query forms remain saturated.
The accuracy builder promised a target false-positive probability but silently clamped a required bit count to the serialization maximum. For sufficiently large inputs it could therefore build a filter that could not meet the requested probability, while still attempting a very large allocation. Compare the calculated bit requirement with the format limit before converting or allocating, and return `InvalidArgument` when the target is not representable. The regression covers both the public suggestion helper and the builder path.
`CpcSketch::validate` and `num_coupons` match supported CPC diagnostics, but the source grouped them as testing methods and did not explain their cost or interpretation. A caller could mistake the coupon count for the cardinality estimate or run validation on a hot path. Document that validation reconstructs a bit matrix proportional to `k`, and that coupons are an internal statistic rather than the distinct-count estimate. This changes documentation only.
`cargo package` warned that the lockfile selected yanked `chacha20` 0.10.1 through `rand` 0.10.2. Shipping a release branch with that warning makes dependency resolution look stale even though the manifest constraint remains valid. Update only the lockfile entry to MSRV-compatible `chacha20` 0.10.2. Manifest requirements and the public dependency graph are unchanged, and package verification completes without the yank warning.
`thetacommon::JaccardSimilarity` is intentionally the shared result type for both Theta and Tuple operators. Re-exporting it from each family module created duplicate public paths and incorrectly privatized a supported namespace. Restore the public `thetacommon` entry point and the original test imports, and remove the migration note from the changelog. Jaccard computation and result representation are unchanged.
These literals are constrained to `usize` by their comparison, collection, return, branch, or function-argument context. Removing the suffixes keeps the expressions shorter without changing their inferred types. Keep explicit suffixes where Rust needs them for method resolution or where the integer width affects shift semantics.
tisonkun
force-pushed
the
codex/library-usability-hardening
branch
from
August 30, 2026 14:35
bfce107 to
a767cb1
Compare
`CountMinSketch::reset` was added for a hypothetical allocation-reuse workload rather than an existing caller, issue, compatibility requirement, or reference implementation. Remove the public method, its dedicated test, and its unreleased changelog entry. A reset contract can be designed when a concrete workload demonstrates that reconstruction is insufficient.
`TDigestMut::reset` was introduced for an assumed window-reuse optimization without a concrete caller, issue, compatibility requirement, or corresponding API in the DataSketches Java and C++ implementations. Remove the public method, its reset-only buffer helper, the dedicated test, and its unreleased changelog entry. Revisit the API only when a real workload can define the required reuse and allocation guarantees.
The coupon counter and full-state validator are exposed so integration tests can inspect CPC invariants. They are not part of the supported query API: Java keeps the coupon count package-private, while C++ marks both hooks as private/debugging helpers. Keep the methods callable for the existing external test crate, but exclude them from generated API documentation.
Saturating signed counters is not a valid overflow policy for a mergeable sketch. It is non-associative when positive and negative weights mix, so update order and merge grouping can change estimates. The saturating absolute value of the minimum signed integer also silently undercounts a magnitude that the value type cannot represent. Restore the previous arithmetic until overflow behavior is designed as an explicit API contract. This reverts commit 88e6ea1.
Saturating u64 counters silently changes the sketch from tracking mathematical stream frequencies to tracking capped frequencies. Once the true count exceeds u64::MAX, the documented upper-bound guarantee can no longer hold even though the returned value looks valid. Restore the previous arithmetic until overflow behavior is defined explicitly by the public API. This reverts commit e49a579.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the crate’s public surface to be safer and more usable as a library by replacing panic-driven contracts with typed Result errors, tightening sizing/validation, and improving cross-language guidance and persistence ergonomics.
Changes:
- Make key public APIs fallible (Bloom union/intersect, Count-Min merge, CPC union update, CPC
max_serialized_bytes, Bloom sizing helpers) and update integration tests accordingly. - Align frequent-items sizing helpers with constructor units, add explicit max-map-size accessors, and validate inputs rather than coercing.
- Improve T-Digest ergonomics (explicit deserialize precision entrypoints, immutable digest serialize/deserialize, split-point empty-slice behavior) plus documentation/changelog updates.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests-integration/tests/tdigest_test/sketch.rs | Adds regression test for empty split-point handling in CDF/PMF. |
| tests-integration/tests/serde_tests/tdigest.rs | Updates deserialization API usage and adds frozen T-Digest roundtrip coverage. |
| tests-integration/tests/frequencies_test/update.rs | Updates helper APIs to max-map-size units; adds invalid-input assertions. |
| tests-integration/tests/cpc_test/wrapper.rs | Adapts CPC union update calls to fallible API. |
| tests-integration/tests/cpc_test/update.rs | Adds validation tests for fallible max_serialized_bytes. |
| tests-integration/tests/cpc_test/union.rs | Updates CPC union tests for fallible update + reduced runtime test workload. |
| tests-integration/tests/countmin_test/sketch.rs | Updates Count-Min merge test to expect typed error instead of panic. |
| tests-integration/tests/bloom_test/sketch.rs | Updates Bloom union/intersect tests to expect typed error + adds validation tests. |
| README.md | Expands cross-language hashing guidance in crate README. |
| datasketches/src/thetafamily/tuple/sketch.rs | Adjusts docs and minor numeric literal cleanup. |
| datasketches/src/thetafamily/tuple/mod.rs | Removes TupleEntry re-export to avoid leaking private storage representation. |
| datasketches/src/tdigest/sketch.rs | Refactors serialization, splits deserialize entrypoints by precision, adds immutable persistence, and fixes empty split-point handling. |
| datasketches/src/req/sketch.rs | Removes hidden diagnostic APIs and minor literal cleanup. |
| datasketches/src/lib.rs | Adds crate-level guidance on features, sketch selection, and cross-language hashing. |
| datasketches/src/frequencies/sketch.rs | Adds max-map-size validation, updates helper APIs to max-map-size units, and exposes configured max-map-size. |
| datasketches/src/frequencies/reverse_purge_item_hash_map.rs | Minor literal cleanup in purge loop. |
| datasketches/src/cpc/union.rs | Changes update to return Result for seed mismatch; removes test-only state leakage. |
| datasketches/src/cpc/sketch.rs | Makes max_serialized_bytes fallible and hides validation helpers from docs. |
| datasketches/src/countmin/sketch.rs | Changes merge to return Result on incompatible configuration. |
| datasketches/src/bloom/sketch.rs | Changes union/intersect and sizing helpers to return Result and validate/limit sizing. |
| datasketches/src/bloom/mod.rs | Updates module docs to reflect fallible union/intersect API. |
| CHANGELOG.md | Documents breaking changes, new TDigest persistence feature, and bug fixes. |
| Cargo.lock | Updates chacha20 lockfile entry. |
| benchmarks/tdigest/serde.rs | Updates benchmark to new TDigest deserialize API. |
| benchmarks/tdigest/merge.rs | Updates benchmark to new TDigest deserialize API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The public formula helpers duplicated the accuracy builder and exposed intermediate values that callers could combine inconsistently.\n\nInline target sizing into build, document validation and rounding at the two construction entry points, and retain only behavior-level boundary coverage.
REQ is new since the latest release, so its intermediate diagnostic methods and try_new migration are not release-facing changes.\n\nRemove those development-only details and consolidate the Count-Min suggestion signature and behavior into one migration entry.
The Count-Min implementation and release notes were already updated together in c25f551. Restore their existing category split so the release-baseline cleanup remains focused on removing development-only REQ history.
Keep the detailed policy in CONTRIBUTING.md and make AGENTS.md point to that section. This avoids duplicated wording drifting between the contributor and agent instructions.
The suggestion methods changed their public return types from integers to Result, so callers must update even though the new validation also fixes invalid outputs. Record the complete final behavior once under breaking changes.
tisonkun
enabled auto-merge (squash)
August 30, 2026 15:48
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
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.
Summary
This PR tightens public API contracts and removes a few accidental implementation leaks:
BloomFilterBuilder, validate target/manual construction before allocation, and return typed errors from incompatible Bloom, Count-Min, and CPC operations;max_map_sizeunits as construction;TDigest, and accept empty CDF/PMF split lists;TupleEntrystorage type;chacha20lockfile version.Migration details for breaking API changes are recorded in
CHANGELOG.md.Validation
cargo x checkcargo x testcargo x lint