fix(query): value-weighted topk, HLL global rollup, range warm+archive stitch - #375
Merged
Merged
Conversation
…e stitch Three correctness fixes in the ASAP query backend, each with a focused unit test (all green; full data_plane lib suite 871 passed / 0 failed). FIX 1 — CountSketch/CMS topk ranked by frequency, recall 0. The heavy-hitter heap was built by `+1`-per-occurrence updates keyed by the raw `item`, so `topk(k, sum by(label)(metric))` ranked groups by occurrence COUNT, not summed VALUE (eval-plan Fig 3c). Add a value-weighted update path on `CountMinSketchWithHeapAccumulator`: `insert_value(group_label, value)` adds the sample value (not +1) keyed by the group label (delegating to the library's value-weighted `CountMinSketchWithHeap::update`), and `topk_by_value(k)` reads the top-k groups ranked by Σvalue. Test crafts an adversarial dataset where the busiest-by-count host is not the heaviest-by-value-sum host and asserts value-weighted top-k recall == 1.0. FIX 2 — HLL global rollup (`count(hll_metric)` no `by`) returned empty / per-series. Add `SketchReducer::evaluate_cardinality_global`, which merges the per-series HLL registers (register-wise max) across all matched sids via the new `delta_apply::cumulative_hll_state` and estimates once — the distinct UNION cardinality, not the sum of per-series estimates. Wired in the instant `execute` path for `CardinalityApprox` + empty group_by + outer Count. Test builds two HLL series with overlapping + disjoint items and asserts the merged global estimate is within HLL error of the true union and well below the naive per-series sum. FIX 3 — Range queries returned warm-only, losing the prefix when warm coverage was narrower than the request. Wire the same coverage-aware warm+archive stitch the instant path uses into `execute_range_promql_modern`: when the reducer reports a coverage narrower than `[start_ms, end_ms]` AND an archive engine is configured, fetch the archive range answer and stitch via `stitch_warm_and_archive` (warm wins on overlap). Test: warm covers only the suffix, asserts the stitched matrix spans the full range (prefix from archive, suffix from warm); plus an archive-less control. Co-Authored-By: Claude Opus 4.8 (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.
Three correctness fixes in the ASAP query backend (
data_plane), each with a focused unit test. Fulldata_planelib suite: 871 passed / 0 failed.FIX 1 — CountSketch/CMS topk ranked by frequency (recall 0)
Root cause: the heavy-hitter heap (
CountMinSketchWithHeapAccumulator) was built by+1-per-occurrence updates keyed by the rawitem, sotopk(k, sum by(label)(metric))ranked groups by occurrence count, not summed value (eval-plan Fig 3c: "value-weighted topk needs a separate update path").Change:
count_min_sketch_with_heap_accumulator.rs—insert_value(group_label, value)adds the sample value (not +1) keyed by the group label (delegates to the library's value-weightedCountMinSketchWithHeap::update);topk_by_value(k)returns the top-k groups ranked by Σvalue.Test:
value_weighted_topk_has_full_recall_vs_count_topk— adversarial dataset where the busiest-by-count host (h_chatty, 100 tiny samples) is not the heaviest-by-value-sum host (h_heavy, few huge samples); asserts value-weighted top-k recall == 1.0 and excludesh_chatty. Plusinsert_value_accumulates_summed_value_in_heap.FIX 2 — HLL global rollup empty / per-series
Root cause:
count(hll_metric)with noby (...)dispatched to the per-series Cardinality path, emitting one estimate per series (double-counting overlaps; never the single global number).Change:
sketch_reducer.rs::evaluate_cardinality_globalmerges per-series HLL registers (register-wise max) across all matched sids via the newdelta_apply::cumulative_hll_state, then estimates once = distinct UNION cardinality. Wired in the instantexecutepath forCardinalityApprox+ emptygroup_by+ outerCount.Test:
execute_count_hll_global_merges_registers_across_series— two HLL series with overlapping + disjoint items; asserts the merged global estimate is within HLL error of the true union (1000) and well below the naive per-series sum (~1200).FIX 3 — Range queries didn't hybrid-stitch warm+archive
Root cause: the instant path stitches warm+archive when warm coverage is narrower than the request, but
execute_range_promql_modernreturned warm-only, losing the prefix[start, cov_lo).Change: wire the same coverage-aware stitch into the range path — when the reducer reports a coverage narrower than
[start_ms, end_ms]AND an archive engine is configured, fetch the archive range answer and stitch viastitch_warm_and_archive(warm wins on overlap).Test:
range_stitches_archive_prefix_with_warm_suffix— warm (CountMincount_over_time) covers only the suffix; asserts the stitched matrix spans the full range (prefix from archive, suffix from warm, warm wins on overlap). Plusrange_warm_only_when_no_archive_enginecontrol.🤖 Generated with Claude Code