Fix three bugs in Nitro geometric row-sampling - #82
Open
zzylol wants to merge 3 commits into
Open
Conversation
1. CountMin::fast_insert_nitro was missing the loop present in
Count::fast_insert_nitro, causing a usize underflow in
`(r + temp + 1) - rows` at full sampling (rate == 1.0), which
silently disabled all further row-touches for the sketch's lifetime.
2. Nitro::draw_geometric (structure_utils.rs) had its live RNG-based
draw commented out and replaced by a fixed ~1%-rate precomputed
table lookup, ignoring the configured sampling_rate entirely at
every other rate.
3. draw_geometric (both structure_utils.rs::Nitro and
sketch_framework/nitro.rs::NitroBatch) generated Geo(p) directly
(support {1,2,...}, mean 1/p), but callers advance the row cursor
via `r += to_skip + 1`, which double-counts the +1 already implied
by the paper's Algorithm 1 (`r += Geo(p)`, r initialized at -1).
This inflated the true step mean to 1/p + 1, so the achieved
sampling density was p/(1+p) instead of p -- e.g. ~33% actual
density at a configured rate of 50%, systematically undercounting
every Nitro estimate. Fixed by drawing Geo(p) - 1 instead.
Verified via an isolated probe inserting a single key 1M times at
rates [1.0, 0.5, 0.1, 0.05, 0.02, 0.01] for both CountMin and Count:
estimates now track the true count within ~1-2.5% at every rate,
versus wildly biased or rate-independent results before. Full test
suite (497 unit + integration + doctests) passes with no regressions.
nitro_estimate (CountMin) queried via hash_for_matrix, which for some (rows, cols) dimensions selects a Packed64 hash layout hashed with hash64_seeded -- a different algorithm than the raw hash128_seeded(0, value) that fast_insert_nitro writes with. Insert and query only happened to agree at dimensions landing in Packed128 mode (where the seed-0 case coincides with hash128_seeded(0, key)), masking the bug at the row/col configuration used by the earlier verification probe. Fixed by adding Vector2D::query_by_row (mirrors update_by_row's exact column extraction) and rewriting nitro_estimate to hash and read the same way fast_insert_nitro writes, for both CountMin and Count (Count's fast_insert_nitro previously had no matching query method at all -- callers had to fall back to the also-hash_for_matrix-based regular estimate()). Also: - Added fast_insert_nitro_many(value, many) to both CountMin and Count (Vector2D<i32> and Vector2D<i64>), compensating by Nitro::scaled_increment(many) instead of the fixed per-occurrence delta -- lets one row-cycled, geometrically-sampled call stand in for a weighted observation (e.g. a metric sample's value) without looping the insert call itself, keeping cost proportional to event count rather than magnitude. - Mirrored all Nitro support (enable_nitro, fast_insert_nitro[_many], nitro_estimate) for Vector2D<i64> storage, for callers whose summed weights can exceed i32::MAX. Verified via the same isolated probe, extended with a rows=3/cols=8 case (forces Packed64 mode, previously broken) and a weighted-insert case for both i32 and i64 storage on CountMin and Count -- all track the true value within sampling noise. Full test suite (497 unit + integration + doctests) still passes.
Nitro::init_nitro (and everything built on it: Vector2D::enable_nitro, CountMin/Count::enable_nitro) always seeds its geometric-skip RNG from OS entropy (new_small_rng()), with no way to inject a caller-supplied seed -- appropriate for production use, but it means a caller that needs deterministic, reproducible runs (e.g. a benchmark harness driven by its own --seed flag) can't get repeatable Nitro sampling decisions at all. Added Nitro::init_nitro_seeded(rate, seed), Vector2D::enable_nitro_seeded, and CountMin/Count::enable_nitro_seeded (both Vector2D<i32> and Vector2D<i64>) as siblings to the existing unseeded versions, seeding via SmallRng::seed_from_u64(seed) instead.
Contributor
|
@zzylol did you run some comprehensive accuracy tests before vs after? |
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
2 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.
Summary
Fixes three independent bugs in Nitro (row-level geometric sampling), found while implementing a downstream demo that inserts via
fast_insert_nitro/NitroBatch::insertand reads back withnitro_estimate/estimate.CountMin::fast_insert_nitrounderflow atrate == 1.0. UnlikeCount::fast_insert_nitro, it was missing the loop that lets a single call touch multiple rows. At full samplingr=0, temp=0always, so(r + temp + 1) - rowsunderflows forrows > 1, permanently disabling further row-touches for the sketch's lifetime.Nitro::draw_geometric(structure_utils.rs) ignored the configuredsampling_rate. Its real RNG-based draw was commented out (// for profiling) and replaced by a fixed ~1%-rate precomputed table lookup, so every rate other than ~0.01 was silently miscalibrated.draw_geometricgeneratedGeo(p)directly (support{1,2,...}, mean1/p), but the callers (fast_insert_nitro,NitroBatch::insert) advance the cursor viar += to_skip + 1. Per NitroSketch paper Algorithm 1 (r += Geo(p),rinitialized at-1), that+1is already the "touch" step and should not be added on top of a fullGeo(p)draw. The double-counted+1inflated the true step mean to1/p + 1, so the achieved sampling density wasp/(1+p)instead ofp(e.g. ~33% actual density at a configured rate of 50%), systematically undercounting every Nitro estimate. Fixed by drawingGeo(p) - 1instead, so the existing+1at call sites is correct.Verification
fast_insert_nitroat rates[1.0, 0.5, 0.1, 0.05, 0.02, 0.01]for bothCountMinandCount. Before the fix, estimates were either flat/rate-independent or off by up to ~33%. After, estimates track the true count within ~1-2.5% at every rate.cargo test: full suite (497 unit tests + integration tests + doctests) passes, including the Nitro-specific accuracy tests (nitro_batch_countmin_error_bound_zipf,nitro_batch_count_error_bound_zipf) -- no regressions.Test plan
cargo buildcargo test(497 passed, 0 failed)CountMinandCount