Skip to content

feat: expose kernel codec accessors and edge sampling - #495

Merged
zzylol merged 1 commit into
mainfrom
feat/kernel-codec-accessors
Oct 1, 2026
Merged

zzylol merged 1 commit into
mainfrom
feat/kernel-codec-accessors

Conversation

@zzylol

@zzylol zzylol commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #488.

Why

ASAPQuery-backend #805 stores Planner kernel states directly and keeps only storage codecs. Five gaps in the kernels forced workarounds there: a copy of the UnivMon accumulator, a msgpack round trip to reach the weighted-frequency kernel, a hand-written mirror of the exact state's serde shape, a clone of each cached sketch per ingest frame, and rejecting every edge-sampled frame (0 < sample_p < 1) because kernels had nowhere to keep p.

What

Each new public API exists for one backend need:

API Backend need
UnivMonAccumulator::from_sketch(UnivMon), sketch(), AggregateCore::estimate for UnivMon Delete the asap_summary_state::univmon shim; encode/restore with sketchlib's codec
WeightedFrequency::algorithm(), shape(), to_bytes(), from_bytes() Drop the msgpack round trip in the codec
ExactAccumulator deserialization validates against its family (serde(try_from)) Drop decode_exact's shape mirror; no new method
AggregateCore::as_any_mut Update cached sketches in place on ingest deltas and read-path merges
from_sketch(sketch, sample_p), sample_p(), merge_sample_p(p) on DDSketch, HLL, Count-Min, Count Sketch Accept sampled edge frames again, persist p, record p of deltas applied in place

How: sample_p

p is kept in a private field, so it is always in (0, 1]. from_sketch rejects 0, values above 1, and NaN. The backend should map the proto3 default 0 to 1 before calling it.

Kernel Scaled by 1/p Unscaled
DDSketch bare PointCount (total count) quantiles
HLL Cardinality, bare PointCount none
Count-Min, Count Sketch query_key none

These rules match the kernels at 76fbbf1 for DDSketch, HLL and Count-Min. Count Sketch had no sample_p there; it gets the same 1/p rule because it is linear.

Merge rules:

  • Equal p values merge and keep that p.
  • 1 merged with a sampled p gives the sampled p. This is the old "prefer sampled" rule, which exists because window-reset and Planner-created bases are unsampled.
  • Two different sampled p values are rejected. The old code silently kept the left operand's p here.

Before this PR

The backend decodes SketchEnvelope{CountMinState, sample_p: 0.25} and fails with "frame is edge-sampled … not supported by Planner kernels".

After this PR

let cms = CountMinSketchAccumulator::from_sketch(decoded, 0.25)?;
cms.query_key(&key)          // stored 10 reads as 40
cms.sample_p()               // 0.25, persisted next to the sketch bytes

Validation

  • cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --no-fail-fast all pass.
  • Written first, these tests failed on the base and pass now: exact decode rejecting mismatched or unsupported families, and the UnivMon readouts.
  • Ported from 76fbbf1: DDSketch raw 10 at p=0.1 reads 100 with quantiles unchanged; HLL reads exactly 4× raw at p=0.25; Count-Min reads 4× raw at p=0.25. Each also has a merge-policy test.
  • New tests cover round trips for the UnivMon adoption and the weighted-frequency bytes, rejection of terminal-mode and invalid UnivMon sketches, and in-place as_any_mut updates.
  • An independent reviewer agent reviewed the change. Its finding that deltas applied in place could not record p led to merge_sample_p.

Backend follow-ups

  • sample_p is private, so the backend's struct literals must switch to ::new or from_sketch.
  • NativeSummaryOutput must implement as_any_mut.
  • Stored bytes must carry p next to inner.to_msgpack(). Until they do, keep rejecting sampled frames.
  • Count reads made directly on inner (e.g. inner.estimate(&str) or row totals) are unscaled. Use query_key(&KeyByLabelValues::new_with_labels(vec![item])) or divide by sample_p().
  • The merge rule assumes p is fixed per series. An adaptive p that changes mid-series gets an error for two different sampled values.

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the feat/kernel-codec-accessors branch from d718cad to b945081 Compare September 30, 2026 18:27
@zzylol
zzylol force-pushed the feat/precompute-raw-sample-input branch from 4b6dffd to f574c00 Compare September 30, 2026 18:27
@zzylol
zzylol force-pushed the feat/kernel-codec-accessors branch from b945081 to aebe2ea Compare September 30, 2026 20:24
@zzylol
zzylol changed the base branch from feat/precompute-raw-sample-input to feat/physical-compile-coverage-3 October 1, 2026 22:48
@zzylol
zzylol force-pushed the feat/kernel-codec-accessors branch from aebe2ea to b8440f7 Compare October 1, 2026 22:48
@zzylol
zzylol force-pushed the feat/physical-compile-coverage-3 branch from 148353e to b2c0aa6 Compare October 1, 2026 22:58
@zzylol
zzylol changed the base branch from feat/physical-compile-coverage-3 to main October 1, 2026 23:06
@zzylol
zzylol force-pushed the feat/kernel-codec-accessors branch from b8440f7 to a9db8d7 Compare October 1, 2026 23:06
@zzylol
zzylol merged commit c3d8f25 into main Oct 1, 2026
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant