Export impl_hll_bucket_list! so callers can build any HLL precision - #95
Open
GordonYuanyc wants to merge 6 commits into
Open
Export impl_hll_bucket_list! so callers can build any HLL precision#95GordonYuanyc wants to merge 6 commits into
GordonYuanyc wants to merge 6 commits into
Conversation
`HllRegisterStorage` types could only come from the three precisions the crate instantiates (lg_k 12/14/16); a caller needing another precision (e.g. lg_k=18) had no way to build one at all. Mark `impl_hll_bucket_list!` `#[macro_export]` so downstream crates can generate their own storage type and keep the same monomorphized fast path. As with `impl_fixed_matrix!`, the expansion is now fully path- qualified (`$crate::__private::serde`, `$crate::HllRegisterStorage`, `::std::ops::*`) so it compiles in a caller that has not imported those names or taken a serde dependency. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`tests/hll_custom_precision.rs` compiles as a separate crate, so it only sees the public surface — invoking `asap_sketchlib::impl_hll_bucket_list!` there is a real check that the export works downstream. Generates storage at lg_k 4, 8, 10, 13, and 18 (the precision sketch-bench's `lkarger` search space needs) and asserts the storage constants, Classic and HIP estimates within tolerance, merge at lg_k=18, index/range/iterator access, and a MessagePack round-trip. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`estimate` for the ErtlMLE variant was generated by `impl_ertl_mle_estimate!` and instantiated only for P12/P14/P16, so a custom-precision storage type could insert and merge an ErtlMLE sketch but never query it. The only thing forcing the macro was the exact-sized histogram, `[u32; REGISTER_BITS + 2]`, which cannot be written in a generic impl while `generic_const_exprs` is unstable. It does not need to be exact: `REGISTER_BITS + 2` peaks at 66, so a single `[u32; 66]` covers every precision and the loop bounds stay `Registers::REGISTER_BITS` — still a compile-time constant in each monomorphization. The slots past the end are zeros the algorithm never reads, so estimates at the existing precisions are bit-identical. Drops the macro and the two `#[cfg(test)]` harness macros that only existed to work around it. Extends the custom-precision test to assert ErtlMLE accuracy alongside Classic and HIP. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Default` built `[0_u8; NUM_REGISTERS]` as a stack value and copied it into the box, and `Deserialize` did the same with `BigArray`'s fixed array. Release builds elide those temporaries, but debug builds do not: on a 2 MiB test thread, deserializing overflowed the stack at lg_k >= 17 and `default()` at lg_k >= 22. That was survivable while the crate only shipped lg_k <= 16 (64 KiB), but exporting the macro makes the affected precisions reachable, and a hard abort is a poor failure mode for `cargo test`. `Default` now goes through a zeroed boxed slice, and `Deserialize` fills a heap-allocated array through a visitor instead of `BigArray::deserialize`. Neither path ever names an `[u8; N]` value, so stack use is constant and no longer depends on the optimizer. The encoding is untouched -- `Serialize` still uses `BigArray`, and the visitor reads the same fixed-length sequence via `deserialize_tuple`. Bytes written by the previous implementation decode to identical registers and re-encode byte-for-byte. Adds a regression test at lg_k 18 and 22; it aborts with a stack overflow without this change. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems a code review turned up, both specific to the macro now being public API. Hygiene: the expansion still named `Result`, `Box`, `Default` and `unreachable!` bare. `Result` is the one that bites -- `use std::io::Result` or `use std::fmt::Result` at module scope is ordinary, and either makes the expansion fail with eight errors, since the aliases take a different number of generic arguments. Qualified those, plus the `Clone`/`Debug` derive paths. Verified against a downstream crate that shadows `Result` two ways and declares its own `Default` trait. Invariant: nothing tied the second and third arguments together, so `impl_hll_bucket_list!(Foo, 18, 1_usize << 20)` compiled and produced a sketch that addresses `1 << 18` buckets while dividing by `1 << 20` -- every estimate silently wrong, no panic, no bounds violation (measured: 83131 of 1048576 registers ever reachable). A `const _: ()` block now asserts `num_registers == 1 << precision` and `precision >= 1` at compile time, and `HllRegisterStorage` documents the same relations for hand-written impls, which the estimators also depend on. The sibling `impl_fixed_matrix!` has the same bare-`Result` flaw. Left alone here: it is pre-existing, unrelated to this issue, and deserves its own pass over all of its unqualified names. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-ups on the test file: - The lg_k=4 tolerance was 1.10, so an estimator returning zero would have passed (relative error 1.0). Tightened to 0.60; the measured worst case at that precision is 0.43, and every tolerance is now below 1.0. - Added an ASAPv1 envelope round-trip for all three variants at a custom precision. That is a different code path from the derive-based serde impls the file already covered -- it writes `precision` into the metadata and validates register length on decode -- and it is the cross-language format, so it is the one worth guarding. Includes HIP, whose running scalars have to survive the round-trip, and a negative case: lg_k=18 bytes must not decode as lg_k=13. - Reworded the module doc, which claimed no serde dependency while calling `rmp_serde`. The real point is that it imports none of the names the macro expansion needs. Also moves the heap-allocation CHANGELOG entry from Added to Fixed, where a bug fix belongs. Refs #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
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.
Closes #94 (part (a) only — see Not in scope below).
What
Six commits:
bd558fc— export the macro. Markimpl_hll_bucket_list!#[macro_export].Downstream crates can now generate an
HllRegisterStoragetype at anyprecision (e.g.
lg_k = 18, which sketch-bench'slkargersearch spaceneeds) instead of being stuck with the built-in
HllBucketListP12/P14/P16.The generated type is the same monomorphized fast path, not a slow fallback.
c614acd— test it.tests/hll_custom_precision.rs.debe577— makeErtlMLE::estimategeneric, so custom precisions canquery, not just insert and merge.
f60317d— heap-allocate register storage, so large precisions don'toverflow the stack in debug builds.
72c190e— harden the macro against caller context (from review): qualifyResult/Box/Default/unreachable!/derive paths, and assertnum_registers == 1 << precisionat compile time.02ec700— tighten the tests (from review): non-vacuous lg_k=4 tolerance,ASAPv1 envelope round-trip for all three variants at a custom precision.
Commits 3 and 4 came out of checking the full variant × operation matrix at a
custom precision. Result after this PR — every cell works:
mergemethod, by designCommit 1 — note on "one line"
#[macro_export]is the one functional line, but it can't ship alone: the macrobody referenced
Serialize,Serializer,Deserialize,Deserializer,serde_big_array::BigArray,Index,IndexMut,RangeandHllRegisterStorageby bare name, so the expansion only compiled inside thismodule. The rest is mechanical path-qualification —
$crate::__private::serde,$crate::HllRegisterStorage,::std::ops::*— exactly the patternimpl_fixed_matrix!already uses (748f179). Without it a downstream callerwould have to import all eight names and take a
serde-big-arraydependency oftheir own.
Also in commit 1: a doc example on the macro (runs as a doctest, i.e. from
outside the crate), a CHANGELOG entry, and one bullet in
docs/features.mdnext to
impl_fixed_matrix!.Commit 3 — ErtlMLE at custom precisions
estimateforErtlMLEcame fromimpl_ertl_mle_estimate!, instantiated onlyfor P12/P14/P16, so a custom-precision sketch could
insertandmergebutestimatedid not exist:The only thing forcing the macro was the exact-sized histogram
[u32; REGISTER_BITS + 2], unwritable in a generic impl whilegeneric_const_exprsis unstable. It doesn't need to be exact:REGISTER_BITS + 2peaks at 66, so one
[u32; 66]covers every precision and the loop bounds stayRegisters::REGISTER_BITS— still a compile-time constant per monomorphization.The extra slots are zeros the algorithm never reads, so estimates at existing
precisions are unchanged. Net −4 lines; also drops the two
#[cfg(test)]harness macros that only existed to work around the same limitation.
Commit 4 — the debug stack overflow
Defaultbuilt[0_u8; N]on the stack and copied it into the box;Deserializedid the same via
BigArray. Release elides those temporaries — debug does not.On a 2 MiB test thread:
default()/new()rmp_serde::from_slicedeserialize_from_bytes(ASAPv1)Not a new bug — it's equally true of the shipped
HllBucketListP16— butexporting the macro makes the precisions where it bites reachable, and
fatal runtime error: stack overflowis a bad first experience for someone whojust generated a P18 type. Fix routes both paths through a heap allocation that
never names an
[u8; N]value, so behavior no longer depends on the optimizer.Wire compatibility: unchanged.
Serializestill usesBigArray; the newvisitor reads the same fixed-length sequence via
deserialize_tuple. Verifieddirectly — bytes written by the pre-change implementation decode to identical
registers under the new one and re-encode byte-for-byte (16388 bytes, P14).
Review follow-ups (commits 5–6)
A review pass turned up two real defects in the exported macro, both reproduced
before fixing and re-verified after:
Unqualified
Resultbroke the expansion in ordinary caller modules. Withuse std::io::Resultoruse std::fmt::Resultin scope — the aliases take adifferent number of generic arguments — the macro failed with 8 errors
(E0053/E0107/E0277/E0308). Commit 1 had qualified
serde,std::ops,vec!and
write!but missedResultin three signatures, plusBox,Defaultand
unreachable!. Now verified against a downstream crate that shadowsResulttwo ways and declares its ownDefaulttrait, underdeny(warnings).Nothing tied
precisiontonum_registers.impl_hll_bucket_list!(Foo, 18, 1_usize << 20)compiled and silently producedwrong estimates forever: the bucket index is
(hash >> REGISTER_BITS) & P_MASK,so only
1 << 18registers are ever reachable whileestimate()divides by1 << 20. Measured on the broken pair: 83,131 of 1,048,576 registers evertouched. No panic, no bounds violation — it just returns bad numbers. A
const _: ()block now rejects it at compile time:precision >= 1is checked the same way, andHllRegisterStoragenowdocuments the relations hand-written impls must uphold.
The review also independently confirmed the PR's three load-bearing claims:
ErtlMLE estimates byte-identical across P12/P14/P16 × 8 cardinalities; MessagePack
output byte-identical (16388 B,
cmpclean, and bytes cross-decode bothdirections); insert throughput unchanged.
Known, deliberately not fixed here:
impl_fixed_matrix!in the same file hasthe identical bare-
Resultflaw (pre-existing, and it is already exported). Itdeserves its own pass over all of its unqualified names rather than a partial fix
smuggled into this PR.
Test coverage
tests/hll_custom_precision.rsis an integration test, so it compiles as aseparate crate against the public surface only — invoking the macro there is
itself the check that the export works downstream. It instantiates lg_k
4, 8, 10, 13, 18, and 22 (the last only in the allocation test) and asserts:
PRECISION,NUM_REGISTERS,REGISTER_BITS,P_MASK,len, zeroed ondefault())over deterministic inputs (so the assertions can't flake); tolerances are ~4x
the 1.04/sqrt(m) standard error, and measured errors sit well inside them
(e.g. lg_k=18: classic 0.01%, ErtlMLE 0.01%, HIP 0.15%)
REGISTER_BITS + 1mergeof two overlapping lg_k=18 sketches recovers the union cardinalityIndex/IndexMutforusizeandRange, andIntoIteratorcustom precision for all three variants — including HIP's running scalars,
plus a negative case (lg_k=18 bytes must not decode as lg_k=13)
cargo testandcargo test --releaseboth fully green (505 unit +6/7/12/4/1 integration + 21 doctests).
cargo fmt --checkandcargo clippy --all-targetsclean.Not in scope
Part (b), the
Vec<u8>runtime-sized fallback, is not here. Worth a separatePR — with (a) in place, every precision is reachable, so (b) becomes a
convenience (no type declaration needed up front) rather than the only way to
run at lg_k=18. Note the portable wire type
message_pack_format::portable::HllSketchalready carries a runtime
precision, so there's prior art for the shape.🤖 Generated with Claude Code