Repository navigation
fix(asap-physical-operators): seed each CountSketch row independently - #706
Merged
Merged
Conversation
Move the executor's pin from e6b0314 to 9415c5b, which merges asap_sketchlib's public WeightedFrequency::estimate and heap-less capacity 0 (04ffc39) into the previous pin. The adapter exposes the estimate as point_estimate.
The plain CountSketch kernel wrapped asap_sketchlib's portable CountSketch, which slices one shared hash across rows: at width 30000 only about four rows are independent, and deeper shapes panic in debug builds. The planner's binomial-tail failure probability assumes independent rows at any depth. The plain CountSketch now builds sketchlib's per-row-seeded weighted frequency kernel in CountSketch mode with no candidate heap, the same kernel the keyed CountSketch-with-heap build uses. Each label value of the key is one Utf8 identity. Plan validation and kernel construction reject an even depth, since the median needs an odd row count. The portable-CountSketch adapter is removed. Tests: a 15 x 30000 shape builds; its rows put keys in pairwise independent columns; every point estimate on a skewed stream is within the certified sqrt(3/w) * L2 bound; merging two panes equals the union build.
…ded kernel create_planner_accumulator built CmsWithHeap and CountSketchWithHeap on asap_sketchlib's portable CMSHeap and CSHeap. Those hash row r with SEEDLIST[r % 20], so row r + 20 copies row r and the planner's depths (23 to 149 rows for CountSketch) are not independent. They also round weights and drop nonpositive ones, and the DAG cannot load their states, since it expects the weighted frequency kernel. Both families, and the plain CountSketch, now build through one WeightedFrequencyUpdater on sketchlib's per-row-seeded weighted frequency kernel, with the DAG keyed build's Utf8 identities and exact Float64 weights. A negative Count-Min input fails validation instead of being dropped. The portable heap updaters stay for callers that construct them directly. Tests: 25-row Count-Min and CountSketch heap states keep every pair of rows independent; factory heap states load as typed DAG summary values and keep fractional weights exactly; a negative Count-Min input is rejected.
asap_sketchlib's KMV reserves all k hash slots at construction, but approx_memory_bytes counted only the hashes held so far, so a fresh or sparse state reported far less than it holds. It now counts k * 8 bytes.
…tSketch depths A CountSketch reads the median of its rows, and the executor builds only odd depths, so evidence measured at an even CountSketch or CountSketch-with-heap depth describes no executable shape. valid_params now rejects it; Count-Min depths stay unrestricted.
zzylol
force-pushed
the
fix/countsketch-row-seeds
branch
from
October 15, 2026 14:25
8cb5bfe to
6cf3454
Compare
zzylol
added a commit
that referenced
this pull request
Oct 15, 2026
…ketch checks, dead portable heap updaters removed (#721) refactor(physical-operators): move the asap_sketchlib pin onto the library's shape, weight and memory checks One workspace pin for every crate: asap-physical-operators inherits the workspace asap_sketchlib (3f44cf1, a pin branch merging the upstream fixes onto 9415c5b) instead of its own rev. The planner stops restating sketch-library rules: - WeightedFrequency::configuration and plan validation of a plain CountSketch ask WeightedFrequency::check_shape (odd depth, positive dimensions); empirical cost evidence does the same. - UnivMonAccumulator, plan validation and the accuracy model's hash_independent ask UnivMon::check_shape, which also carries the 20-row wire bound the accuracy model did not check. - KMV reads its estimate and merges by reference and reports its own memory, so the adapter no longer clones a state to read or merge it and no longer guesses k * 8 bytes. refactor(physical-operators): thin the frequency updater and delete dead portable sketch workarounds WeightedFrequencyUpdater keeps only its state: it validates a sample with the kernel's check_weight, resets with clear, and keys a label vector with update_labels, one String per label instead of an Arc<str> and a String. Removed, since no workspace code builds them after #706 moved the factory to the per-row-seeded weighted frequency kernel: - CmsHeapAccumulatorUpdater, CountSketchWithHeapAccumulatorUpdater and TopkWeight (factory.rs); - CountMinSketchWithHeapAccumulator (count_min_sketch_with_heap.rs) and CountSketchWithHeapAccumulator (count_sketch_with_heap.rs), which wrapped sketchlib's integer heaps that round and drop weights. DatasketchesKLLAccumulator drops its unsafe Send and Sync impls: asap_sketchlib's KllSketch is already Send and Sync. test(aware-mapping): a 21-row UnivMon certifies no guarantee refactor(physical-operators): drop the KLL safety comment left by the removed unsafe impls
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.
Why
The planner certifies a CountSketch's point-query failure probability with the exact binomial tail
P(Bin(depth, p) >= (depth+1)/2)(#702), and Count-Min's with the matching per-row bound. Both assume the rows hash independently.factory::create_planner_accumulatordid not build independent rows:CountSketchwrappedasap_sketchlib's portableCountSketch, which slices one shared Packed64 hash across rows. At width 30000 only about four rows get independent hash bits, and deep shapes panic in debug builds.CmsWithHeapandCountSketchWithHeapwrapped the portableCMSHeap/CSHeap, which hash rowrwithSEEDLIST[r % 20]. Rowr + 20copies rowr, so the planner's CountSketch depths (23 to 149) are not independent. These updaters also rounded weights and dropped nonpositive ones, and their states could not be loaded by the DAG executor, whosevalidate_stateexpects the weighted frequency kernel.What
CountSketch,CmsWithHeapandCountSketchWithHeapthrough oneWeightedFrequencyUpdater. It wrapsasap_sketchlib's per-row-seeded weighted frequency kernel (controller-OMP #160 upstream), the kernel the DAG's keyed summary build already uses. Plain CountSketch uses capacity 0 (no candidate heap). Rows are pairwise independent at any depth, weights are exact Float64 values, and the states are the DAG's state type.validate_single_inputinstead of being dropped. CountSketch accepts signed weights.asap-aware-mappingempirical_cost::valid_params), because the median needs an odd row count. The planner'scount_sketch_depthalready picks odd depths.summary_kernels/count_sketch.rs,CountSketchAccumulator) is removed. The portable heap updaters (CmsHeapAccumulatorUpdater,CountSketchWithHeapAccumulatorUpdater) and accumulators stay public for callers that construct them directly, but the Planner factory no longer uses them.approx_memory_bytesnow countsk * 8bytes, sinceasap_sketchlibreserves allkhash slots at construction.asap_sketchlibpin forasap-physical-operators:e6b0314->9415c5b(still withfeatures = ["experimental"]for KMV). The new pin ise6b0314merged with upstream commit04ffc39, which adds a publicWeightedFrequency::estimateand allows capacity 0. The adapter exposes the estimate asWeightedFrequency::point_estimate.How
Each label value of the key is one
Utf8identity, as in the DAG's keyed build, and each raw sample's value is the weight.WeightedFrequency::configurationmaps aCmsWithHeap/CountSketchWithHeapfamily to its(algorithm, width, depth, capacity), so the factory andvalidate_stateagree on the shape. Panes merge cell by cell, because every pane seeds rowrthe same way.Plain
CountSketchonly reaches this factory path: the DAG executor has no native plain-CountSketch state, so such a family cannot bind there.Key code interfaces added / modified
crates/asap-physical-operators/src/summary_kernels/factory.rscrates/asap-physical-operators/src/summary_kernels/weighted_frequency.rs,mod.rscrates/asap-physical-operators/src/capability.rs,summary_kernels/kmv.rscrates/asap-aware-mapping/src/empirical_cost.rscrates/asap-physical-operators/Cargo.tomlUsage, end to end (a top-k over raw samples; the numbers come from a scratch test on
precompute_raw_samples.rs's harness, run atcebffbdd):Before this PR
After this PR
Every factory-built frequency sketch has independent rows at any odd depth, keeps weights exactly, and loads as a DAG summary value. The planner's δ certification describes what the executor runs.
Verification
New tests:
deployment::deep_plain_count_sketch_rows_hash_independently: a 15 x 30000 plain CountSketch builds. For 2000 keys each key touches exactly one cell per row, and every pair of rows puts at most 5 keys in the same column (about 0.07 expected under independence). Keys are packed 33 per state with weights3^p, so each cell's balanced-ternary digits name the keys that hit it.deployment::deep_heap_sketch_rows_hash_independently: the same check for 25 x 30000CmsWithHeapandCountSketchWithHeapover 1000 keys (past the 20-row seed list).deployment::heap_sketch_states_load_as_dag_summary_values: factory heap states load into aBatchas typed summary values, and weights0.25 + 0.5estimate exactly0.75.deployment::count_min_heap_rejects_negative_inputsdeployment::deep_plain_count_sketch_estimates_meet_the_certified_bound: on a Zipf-like stream over 3000 keys, every point estimate is withinsqrt(3/w) * ||f||_2.deployment::plain_count_sketch_merge_equals_union_build: merged cells and estimates equal the union build.plan_properties::count_sketch_kernel_requires_an_odd_depthandempirical_cost::tests::count_sketch_evidence_requires_an_odd_depthkmv::tests::memory_counts_the_k_reserved_hashesThe heap tests, the odd-depth evidence test and the KMV memory test were run and failed before their fixes.
Commands run on the PR head:
cargo nextest run -p asap-physical-operators: 205 passed, 3 skipped (already#[ignore]d).cargo nextest run -p asap-aware-mapping: 530 passed.cargo nextest run -p asap-integration-tests: 245 passed, 33 skipped (already#[ignore]d).cargo fmt --all -- --checkandcargo clippy --workspace --all-targets --all-features --locked -- -D warnings: clean.Screenshots and measurements: not applicable.
Limitations and follow-ups
summary_kernels::CountSketchAccumulator(otel.rs), which this PR removes. It will need an update when it bumps its ASAPPlanner pin.9415c5bsits on the planner pin branch inasap_sketchlib. Upstream commit04ffc39(branchfeat/weighted-frequency-point-estimate) is in feat(weighted_frequency): public point estimate and heap-less capacity 0 asap_sketchlib#225, still open.